Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

M1eyu2018
Copy link

@M1eyu2018 M1eyu2018 commented Apr 11, 2024

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: #


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.

@M1eyu2018 M1eyu2018 force-pushed the fix_ensemble_change branch 2 times, most recently from 71aa7e4 to e815c47 Compare April 11, 2024 08:29
@M1eyu2018 M1eyu2018 changed the title fix: Invoke sendAddSuccessCallbacks if no bookie will be replaced in ensemble change fix: No need to triggerLoop if all failed bookies are not in current ensemble Apr 11, 2024
@M1eyu2018 M1eyu2018 changed the title fix: No need to triggerLoop if all failed bookies are not in current ensemble fix: No need to trigger ensembleChangeLoop if all failed bookies are not in current ensemble Apr 11, 2024
@jiazhai jiazhai requested a review from hangc0276 April 11, 2024 09:16
Copy link
Contributor

@hangc0276 hangc0276 left a 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.

} else {
nettyOpLogger.registerFailedEvent(MathUtils.elapsedNanos(startTime), TimeUnit.NANOSECONDS);
}

@horizonzy tested this solution, and it works.

@horizonzy
Copy link
Member

#4285 can fix the problem, but this PR is also good for avoiding the unnecessary ensembleChangeLoop. I think this is useful.

@M1eyu2018 M1eyu2018 force-pushed the fix_ensemble_change branch 4 times, most recently from 5901809 to 23f0c51 Compare April 15, 2024 03:22
@M1eyu2018
Copy link
Author

M1eyu2018 commented Apr 15, 2024

@hangc0276 @horizonzy I have added a unit for this PR. PTAL

@lhotari
Copy link
Member

lhotari commented Apr 16, 2024

I this related to #4194? /cc @shustsud

Copy link
Member

@shoothzj shoothzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shoothzj
Copy link
Member

shoothzj commented Jul 8, 2024

@dlg99 @hangc0276 @eolivelli PTAL, thanks

Integer bookieIndex = entry.getKey();
BookieId addr = entry.getValue();
if (origEnsemble.get(bookieIndex).equals(addr)) {
changingEnsemble = true;
Copy link
Contributor

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.

@hangc0276
Copy link
Contributor

@M1eyu2018 Thanks for your contribution. Would you please help check the failed tests? Thanks.

@shoothzj
Copy link
Member

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.

@shoothzj shoothzj reopened this Jul 19, 2024
@shoothzj shoothzj mentioned this pull request Jul 19, 2024
@M1eyu2018
Copy link
Author

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.

@M1eyu2018
Copy link
Author

M1eyu2018 commented Jul 19, 2024

@hangc0276 @shoothzj
I have checked the failed tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants