-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix][broker] Fix maxTopicsPerNamespace might report a false failure #24560
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 maxTopicsPerNamespace might report a false failure #24560
Conversation
lhotari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, good work @BewareMyPower
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #24560 +/- ##
============================================
+ Coverage 73.57% 74.33% +0.76%
- Complexity 32624 32973 +349
============================================
Files 1877 1876 -1
Lines 139502 146276 +6774
Branches 15299 16774 +1475
============================================
+ Hits 102638 108739 +6101
+ Misses 28908 28899 -9
- Partials 7956 8638 +682
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I see 4.0.6 has not started the release, change the release label to 4.0.6 |
…pache#24560) (cherry picked from commit 1c5c765)
…pache#24560) (cherry picked from commit 1c5c765)
…pache#24560) (cherry picked from commit 1c5c765) (cherry picked from commit 3bd2794)
…pache#24560) (cherry picked from commit 1c5c765) (cherry picked from commit 125a33f)
…pache#24560) (cherry picked from commit 1c5c765) (cherry picked from commit 125a33f)
…pache#24560) (cherry picked from commit 1c5c765) (cherry picked from commit 125a33f)
…pache#24560) (cherry picked from commit 1c5c765) (cherry picked from commit 3bd2794)
Motivation
The
createIfMissingargument only represents the topic can be created automatically when it does not exist, but it might not trigger the topic creation in thisgetTopiccall. Hence, we cannot simply pass 1 as the 2nd argument here:pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Line 1790 in d95cf3f
Assuming
maxTopicsPerNamespaceis 10, then the following two cases will both fail.Case 1:
Case 2:
Modifications
Add an overload of
checkMaxTopicsPerNamespace, which verifies if the topic exists first. Only call the other overload when the topic does not exist. IncreatePersistentTopic0, call this new overload. ImprovetestMaxTopicsPerNamespaceto verify this change.Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: