-
Notifications
You must be signed in to change notification settings - Fork 904
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: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble #4278
base: master
Are you sure you want to change the base?
Conversation
71aa7e4
to
e815c47
Compare
e815c47
to
452d682
Compare
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.
To fix this issue, check the delayedWriteFailedBookies before replace can solve this problem. But it is better to failed the write request when the write request received failure callback.
bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
Lines 1210 to 1212 in e6b4154
} else { | |
nettyOpLogger.registerFailedEvent(MathUtils.elapsedNanos(startTime), TimeUnit.NANOSECONDS); | |
} |
@horizonzy tested this solution, and it works.
#4285 can fix the problem, but this PR is also good for avoiding the unnecessary ensembleChangeLoop. I think this is useful. |
5901809
to
23f0c51
Compare
@hangc0276 @horizonzy I have added a unit for this PR. PTAL |
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
@dlg99 @hangc0276 @eolivelli PTAL, thanks |
Integer bookieIndex = entry.getKey(); | ||
BookieId addr = entry.getValue(); | ||
if (origEnsemble.get(bookieIndex).equals(addr)) { | ||
changingEnsemble = true; |
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.
We need to set changingEnsemble = true
when entering the else code block to avoid a potential race condition. If all the bookies in the toReplace set do not exist in the current ensemble, set changingEnsemble back to false.
What's more, please add a comment to explain why we need this logic. thanks.
@M1eyu2018 Thanks for your contribution. Would you please help check the failed tests? Thanks. |
Sorry I didn't remember why I close this last week. It maybe a mis-operation. @M1eyu2018 Would you please help check the failed tests? Thanks. |
OK, I will check as soon as possible. |
…n current ensemble Signed-off-by: M1eyu2018 <857037797@qq.com>
23f0c51
to
d574162
Compare
@hangc0276 @shoothzj |
Descriptions of the changes in this PR:
No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble.
Motivation
Fix the bug issue.
#4261
Changes
No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble.
Master Issue: #