-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix][broker] Fix markDeletedPosition race condition in ManagedLedgerImpl.maybeUpdateCursorBeforeTrimmingConsumedLedger() method #25110
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
base: master
Are you sure you want to change the base?
Conversation
|
@oneby-wang Please add the following content to your PR description and select a checkbox: |
f0619ec to
3de1d97
Compare
bbc81d2 to
3569bde
Compare
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #25110 +/- ##
============================================
+ Coverage 74.44% 74.46% +0.01%
- Complexity 34046 34050 +4
============================================
Files 1899 1899
Lines 149655 149673 +18
Branches 17393 17395 +2
============================================
+ Hits 111412 111447 +35
+ Misses 29368 29364 -4
+ Partials 8875 8862 -13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/pulsarbot rerun-failure-checks |
|
Doesn't solve Seems we must ensure some happens-before order between |
daa66dc to
0f89953
Compare
…rsorBeforeTrimmingConsumedLedger
…d testFlushCursorAfterIndividualDeleteInactivity test
…ursorLedgerFailed test
…ursorLedgerFailed test
…erties in ManagedLedgerTest.testTrimmerRaceCondition
…Impl.internalAsyncMarkDelete
afdba8a to
bd62af6
Compare
…eleteEntryToLatest, add test
Motivation
We do mark deleting operation in
ManagedLedgerImpl.maybeUpdateCursorBeforeTrimmingConsumedLedger()after #25087.Mark deleting an already mark-deleted position will throw an Exception.
Please see comment for race condition case: #25101 (comment).
See also:
Modifications
Modify
ManagedLedgerImpl.maybeUpdateCursorBeforeTrimmingConsumedLedger()method:a. snapshot positions into a local variable to avoid race condition.
b. use
markDeletePositioninstead ofpersistentMarkDeletedPositionto setlastAckedPosition, becausepersistentMarkDeletedPositionis updated aftermarkDeletePosition.c. add if check logic to avoid mark deleting an already mark-deleted position.
d. wait
maybeUpdateCursorBeforeTrimmingConsumedLedger()completed when opening ledger.e. move
maybeUpdateCursorBeforeTrimmingConsumedLedger()into synchronized block when creating a new ledger. I think new ledger's initialization work should be completed before the first entry is added to this ledger.Modify
ManagedCursorImpl.getNumberOfEntries()method: snapshot positions into a local variable to avoid race condition, error logs like this:Ignore changes in
ManagedCursorImpl.asyncMarkDelete()method, I just snapshot positions into a local variable to avoid race condition. It should be fixed by: [fix][broker] Move newPosition to nextValidLedger:-1 to avoid cursor position and ledger inconsistency in ManagedCursorImpl #25117.Add
testNeverThrowsMarkDeletingMarkedPositionInMaybeUpdateCursorBeforeTrimmingConsumedLedgertest inManagedLedgerImplTestto verify the code change.Fix tests due to this PR's code change.
Fix some flaky tests introduced by PR [fix][broker] Fix cursor position persistence in ledger trimming #25087.
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: oneby-wang#18