You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "M1eyu2018 (via GitHub)" <gi...@apache.org> on 2024/04/11 03:56:30 UTC
[PR] fix: Invoke sendAddSuccessCallbacks if no bookie will be replaced in ensemble change [bookkeeper]
M1eyu2018 opened a new pull request, #4278:
URL: https://github.com/apache/bookkeeper/pull/4278
Descriptions of the changes in this PR:
Invoke sendAddSuccessCallbacks if no bookie will be replaced in ensemble change.
### Motivation
Fix the bug issue.
https://github.com/apache/bookkeeper/issues/4261
### Changes
Invoke sendAddSuccessCallbacks if no bookie will be replaced in ensemble change.
Master Issue: #<master-issue-number>
> ---
> In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
> checks for pull requests. A pull request can only be merged when it passes precommit checks.
>
> ---
> Be sure to do all of the following to help us incorporate your contribution
> quickly and easily:
>
> If this PR is a BookKeeper Proposal (BP):
>
> - [ ] Make sure the PR title is formatted like:
> `<BP-#>: Description of bookkeeper proposal`
> `e.g. BP-1: 64 bits ledger is support`
> - [ ] Attach the master issue link in the description of this PR.
> - [ ] Attach the google doc link if the BP is written in Google Doc.
>
> Otherwise:
>
> - [ ] Make sure the PR title is formatted like:
> `<Issue #>: Description of pull request`
> `e.g. Issue 123: Description ...`
> - [ ] Make sure tests pass via `mvn clean apache-rat:check install spotbugs:check`.
> - [ ] Replace `<Issue #>` in the title with the actual Issue number.
>
> ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "M1eyu2018 (via GitHub)" <gi...@apache.org>.
M1eyu2018 commented on PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#issuecomment-2055889797
@hangc0276 @horizonzy I have added unit for this PR. PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "shoothzj (via GitHub)" <gi...@apache.org>.
shoothzj closed pull request #4278: fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble
URL: https://github.com/apache/bookkeeper/pull/4278
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "shoothzj (via GitHub)" <gi...@apache.org>.
shoothzj commented on PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#issuecomment-2237924500
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "M1eyu2018 (via GitHub)" <gi...@apache.org>.
M1eyu2018 commented on PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#issuecomment-2237934156
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#discussion_r1668250553
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##########
@@ -2175,14 +2175,27 @@ void handleBookieFailure(final Map<Integer, BookieId> failedBookies) {
if (changingEnsemble) {
delayedWriteFailedBookies.putAll(failedBookies);
} else {
- changingEnsemble = true;
- triggerLoop = true;
-
toReplace = new HashMap<>(delayedWriteFailedBookies);
delayedWriteFailedBookies.clear();
toReplace.putAll(failedBookies);
origEnsemble = getCurrentEnsemble();
+
+ for (Map.Entry<Integer, BookieId> entry : toReplace.entrySet()) {
+ Integer bookieIndex = entry.getKey();
+ BookieId addr = entry.getValue();
+ if (origEnsemble.get(bookieIndex).equals(addr)) {
+ changingEnsemble = true;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#issuecomment-2058822316
I this related to #4194? /cc @shustsud
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "shoothzj (via GitHub)" <gi...@apache.org>.
shoothzj commented on PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#issuecomment-2212913468
@dlg99 @hangc0276 @eolivelli PTAL, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "M1eyu2018 (via GitHub)" <gi...@apache.org>.
M1eyu2018 commented on PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#issuecomment-2238318500
@hangc0276 @shoothzj
I have check the failed tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#issuecomment-2050190050
#4285 can fix the problem, but this PR is also good for avoiding the unnecessary ensembleChangeLoop. I think this is useful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble [bookkeeper]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #4278:
URL: https://github.com/apache/bookkeeper/pull/4278#issuecomment-2213425527
@M1eyu2018 Thanks for your contribution. Would you please help check the failed tests? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org