[fix][broker] fix broker may lost rack information #23331
+12
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lhotari
requested review from
lhotari,
merlimat,
rdhabalia,
hangc0276,
dlg99,
eolivelli and
codelipenghui
15 tasks
lhotari
approved these changes
Sep 23, 2024
lhotari
reviewed
Sep 23, 2024
Comment on lines
+185
to
+190
| .thenAcceptAsync(bookies -> | ||
| writableBookiesWatchers.forEach(w -> w.onBookiesChanged(bookies)), executor); |
There was a problem hiding this comment.
It would be useful to add a comment why all watchers are triggered when a new listener is added.
lhotari
reviewed
Sep 23, 2024
Comment on lines
+198
to
+207
| .thenAcceptAsync(bookies -> | ||
| readOnlyBookiesWatchers.forEach(w -> w.onBookiesChanged(bookies)), executor); |
There was a problem hiding this comment.
It would be useful to add a comment why all watchers are triggered when a new listener is added.
lhotari
added
the
release/blocker
TakaHiR07
force-pushed
the
fix_lost_rack_info_issue
branch
from
aa7fcdc
to
46ea59b
Compare
Technoboy-
approved these changes
Sep 24, 2024
lhotari
added
release/3.3.3
release/3.0.8
and removed
release/3.3.2
release/3.0.7
labels
|
closing and reopening to trigger CI |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #23331 +/- ##
=============================================
- Coverage 73.57% 42.56% -31.02%
+ Complexity 32624 16836 -15788
=============================================
Files 1877 1812 -65
Lines 139502 154517 +15015
Branches 15299 18409 +3110
=============================================
- Hits 102638 65766 -36872
- Misses 28908 80868 +51960
+ Partials 7956 7883 -73
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main Issue: #23330
Relevant issue: #23282, #23283
Motivation
Fix another issue cause broker lost rack information. There is a previous issue also cause broker lost rack information.
Modifications
This is the first potential fix, when construct bookieClient and do watchWritableBookies() to register listener, trigger each listener in writableBookiesWatchers.
This fix can ensure multiple listeners is trigger in a sync way, but it would bring duplicate trigger problem. Since watchWritableBookies() is only executed twice when bookieClient construct. I think the duplicate trigger problem is acceptable.
Alternative fix is to make BookieRackAffinityMapping#watchAvailableBookies become sync method. Change to registrationClient.watchWritableBookies(......).get() in BookieRackAffinityMapping#watchAvailableBookies. Now it is async.
Verifying this change
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: