Issue #3005: Fix ack broken entry succeed in ensemble change unsetting #3041
Conversation
This file contains hidden or 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
Member
Author
|
@fpj @sijie @ivankelly Could you please take time to evaluate this ? |
hangc0276
requested review from
dlg99,
eolivelli,
hangc0276,
hezhangjian,
nicoloboschi and
zymap
StevenLuMT
reviewed
Jul 27, 2022
| // completes. | ||
| // | ||
| // We call sendAddSuccessCallback after unsetting all pending adds to cover this case. | ||
| sendAddSuccessCallbacks(); |
Member
There was a problem hiding this comment.
for multiple pendingAddOps requests, just call sendAddSuccessCallbacks once time?
is it right？ @kezhuw
Member
Author
There was a problem hiding this comment.
sendAddSuccessCallbacks has loop itself, it is unnecessary to call it multiple times after we unset all pending operations.
The keypoint this pr trying to solve is that eager sendAddSuccessCallbacks per pendingAddOp could make later pendingAddOps, which have not been unset, complete spuriously.
Member
|
rerun failure checks or rebase the master new code,it fix some failed checks |
kezhuw
force-pushed
the
fix-ack-broken-entry-succeed-in-ensemble-changing-unset
branch
from
8ae5036 to
5c1014a
Compare
Member
|
fix old workflow,please see #3455 for detail |
Currently, in `LedgerHandle.unsetSuccessAndSendWriteRequest`, `LedgerHandle.sendAddSuccessCallbacks` could be called by `PendingAddOp.unsetSuccessAndSendWriteRequest` **before** all pending adds evaluated. This will make entries which met ack requirement in old ensemble but have not evaluated yet succeed in new ensemble. Fixes apache#3005.
kezhuw
force-pushed
the
fix-ack-broken-entry-succeed-in-ensemble-changing-unset
branch
from
5c1014a to
fa22562
Compare
Member
|
rerun failure checks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Descriptions of the changes in this PR:
Motivation
Currently, in
LedgerHandle.unsetSuccessAndSendWriteRequest,LedgerHandle.sendAddSuccessCallbackscould be called byPendingAddOp.unsetSuccessAndSendWriteRequestbefore all pendingadds evaluated. This will make entries which met ack requirement in old
ensemble but have not evaluated yet succeed in new ensemble.
Changes
Check succeeded entries after unsetting all pending entries in ensemble change unsetting.
Master Issue: #3005
Further thoughts
PendingAddOp.writeCompletecallsLedgerHandle.sendAddSuccessCallbacksincompletedbranch.Currently,
LedgerHandle.sendAddSuccessCallbackswill not be called if failed bookies overlap with all pending write ensembles. So, the code inPendingAddOp.writeCompletesounds help.But after changed,
LedgerHandle.sendAddSuccessCallbackswill be called unconditionally after all pending adds complete unsetting. So I think the snippet inPendingAddOp.writeCompletedoes not help anymore. Can it be dropped ? I am not that confident. Post my thoughts here for evaluation.