-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][broker] Fix currently client retries until operation timeout if the topic does not exist #23530
[fix][broker] Fix currently client retries until operation timeout if the topic does not exist #23530
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23530 +/- ##
============================================
+ Coverage 73.57% 74.36% +0.78%
- Complexity 32624 34427 +1803
============================================
Files 1877 1943 +66
Lines 139502 147045 +7543
Branches 15299 16205 +906
============================================
+ Hits 102638 109346 +6708
- Misses 28908 29274 +366
- Partials 7956 8425 +469
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@BewareMyPower There's a problem in cherry-picking to branch-3.3 and branch-3.0 since TokenAuthenticatedProducerConsumerTest fails. reproducing issue: mvn -DredirectTestOutputToFile=false -DtestRetryCount=0 test -pl pulsar-broker -Dtest=TokenAuthenticatedProducerConsumerTest
It seems to be some sort of isolation problem since the test passes when the single method mvn -DredirectTestOutputToFile=false -DtestRetryCount=0 test -pl pulsar-broker -Dtest=TokenAuthenticatedProducerConsumerTest#testTopicNotFound @BewareMyPower do you have a chance to fix the test in branch-3.3 and also cherry-pick this PR to branch-3.0 after addressing the problem? thanks |
Okay, I will take a look tomorrow. |
@lhotari It's weird that this test should also fail with the master branch. But the CI is not broken |
Motivation
#23291 introduces a regression that when the authentication is enabled, if the topic does not exist and cannot be automatically created (e.g. namespace does not exist), broker will respond with a
ServerError.MetadataError
that is converted to a trivialPulsarClientException
, which is not retriable. In this case, the client will loop in sending thePartitionMetadata
request and failing until the operation timeout happens (30 seconds by default)Besides,
PulsarClientImpl#checkPartitions
is incorrect. Even ifforceNoPartitioned
is false,checkPartitions.complete(0)
will still be called.Because
a && b || c || d
is true whena
andb
are false whilec
is true. Without this fix, the producer creation on a topic whose namespace does not exist will fail withAuthorizationError
because it will go to the topic lookup phase.This is not an expected result because the case that the error is not retriable and the timeout error is confusing to users.
Modifications
ServerError.TopicNotFound
whenisTopicOperationAllowed
fails with HTTP 404 error.PulsarClientImpl#checkPartitions
.testTopicNotFound
to cover the fixes above.testNamespaceNotExist
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: