Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][broker] Unreasonable AuthenticationException reference #18502

Merged
merged 1 commit into from
Nov 18, 2022
Merged

[improve][broker] Unreasonable AuthenticationException reference #18502

merged 1 commit into from
Nov 18, 2022

Conversation

zzccctv
Copy link
Contributor

@zzccctv zzccctv commented Nov 16, 2022

Motivation

Unreasonable AuthenticationException reference causes processing logic redundancy

Modifications

Delete unreasonable exception references

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions
Copy link

@zzccctv Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@zzccctv zzccctv changed the title Unreasonable AuthenticationException reference causes processing logi… [improve][broker] Unreasonable AuthenticationException reference Nov 16, 2022
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Nov 16, 2022
@Technoboy- Technoboy- added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use area/broker ready-to-test labels Nov 17, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

Merging #18502 (57b0574) into master (fdf86d3) will increase coverage by 1.55%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18502      +/-   ##
============================================
+ Coverage     45.67%   47.22%   +1.55%     
- Complexity    10075    10448     +373     
============================================
  Files           693      697       +4     
  Lines         67940    68015      +75     
  Branches       7273     7285      +12     
============================================
+ Hits          31030    32122    +1092     
+ Misses        33333    32307    -1026     
- Partials       3577     3586       +9     
Flag Coverage Δ
unittests 47.22% <ø> (+1.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...java/org/apache/pulsar/proxy/stats/TopicStats.java 58.82% <0.00%> (-41.18%) ⬇️
...g/apache/pulsar/broker/service/StreamingStats.java 75.67% <0.00%> (-8.11%) ⬇️
...ava/org/apache/pulsar/utils/StatsOutputStream.java 65.45% <0.00%> (-5.46%) ⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java 59.75% <0.00%> (-3.31%) ⬇️
...sar/broker/service/schema/SchemaRegistryStats.java 72.50% <0.00%> (-2.50%) ⬇️
.../org/apache/pulsar/client/impl/ConnectionPool.java 37.43% <0.00%> (-1.03%) ⬇️
...a/org/apache/pulsar/proxy/server/ProxyService.java 80.00% <0.00%> (-0.94%) ⬇️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 22.26% <0.00%> (-0.70%) ⬇️
...broker/service/schema/BookkeeperSchemaStorage.java 73.92% <0.00%> (-0.31%) ⬇️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 14.84% <0.00%> (-0.26%) ⬇️
... and 82 more

@StevenLuMT
Copy link
Member

/pulsarbot run-failure-checks

@AnonHxy AnonHxy added this to the 2.12.0 milestone Nov 17, 2022
@zzccctv zzccctv requested review from HQebupt and Technoboy- and removed request for HQebupt and Technoboy- November 17, 2022 08:40
@Technoboy- Technoboy- merged commit e5e5852 into apache:master Nov 18, 2022
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Nov 18, 2022
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreasonable AuthenticationException reference causes processing logic redundancy

I do not think authenticationException was creating redundancy. With this PR merged, we will not longer get helpful stack traces in cases where we easily could. Further, it'll be harder to debug certain authentication exceptions.

@zzccctv - are you able to explain further how this code was unreasonable?

In order to progress the conversation, I created a PR to revert this commit #18529. We don't have to merge it if we can show that it is really valuable, but I would like additional context before closing it. Thanks!

@michaeljmarshall
Copy link
Member

I misread the original code. Please see #18529 for context.

michaeljmarshall pushed a commit to michaeljmarshall/pulsar that referenced this pull request Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants