-
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 topic policies cannot be queried with extensible load manager #23326
[fix][broker] Fix topic policies cannot be queried with extensible load manager #23326
Conversation
|
I skipped |
1231dc1
to
b601f85
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #23326 +/- ##
============================================
+ Coverage 73.57% 74.65% +1.08%
- Complexity 32624 34409 +1785
============================================
Files 1877 1929 +52
Lines 139502 145158 +5656
Branches 15299 15884 +585
============================================
+ Hits 102638 108374 +5736
+ Misses 28908 28530 -378
- Partials 7956 8254 +298
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Motivation
#23319 brings a regression that topic policies cannot be queried with extensible load manager.
compactionScheduleTest
could not succeed now.In
SystemTopicBasedTopicPoliciesService#getTopicPoliciesAsync
, it skips getting events from__change_events
because the reader will fail to create if the load manager does not start.pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
Lines 253 to 259 in 4b3b273
However, the state validation in
ServiceUnitStateChannelImpl
here is wrong:pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java
Line 270 in 4b3b273
The method above returns true when the state is
Closed
,Constructed
orLeaderElectionServiceStarted
, which is the opposite of the expected result (Started
).Modifications
Fix the
ServiceUnitStateChannelImpl#started()
method with the correct state validation.However, if only with this simple fix, the extensible load manager would fail to start. It's because after the table view creation for
loadbalancer-broker-load-data
andloadbalancer-top-bundles-load-data
topics happen after the load manager start. InisAllowAutoSubscriptionCreationAsync
, it will wait until the future ofgetTopicPoliciesAsync
on these topics is done, while the reader creation on__change_events
will fail.Therefore, to fix this start failure, just return true for internal topics in
isAllowAutoSubscriptionCreationAsync
.Verifying this change
ExtensibleLoadManagerImplTest#compactionScheduleTest
will pass now. (It's in the flaky test suite)Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: