[fix][broker] Fix markDeletedPosition race condition in ManagedLedgerImpl.maybeUpdateCursorBeforeTrimmingConsumedLedger() method

[oneby-wang]

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:

1. [fix][test] Fix ManagedCursorTest and NonDurableCursorTest flaky tests #25101
2. [fix][test] Fix ManagedCursorTest and NonDurableCursorTest flaky tests #25101 (comment)

Modifications

1. Modify ManagedLedgerImpl.maybeUpdateCursorBeforeTrimmingConsumedLedger() method:
   a. snapshot positions into a local variable to avoid race condition.
   b. use markDeletePosition instead of persistentMarkDeletedPosition to set lastAckedPosition, because persistentMarkDeletedPosition is updated after markDeletePosition.
   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.

2. Modify ManagedCursorImpl.getNumberOfEntries() method: snapshot positions into a local variable to avoid race condition, error logs like this:

java.lang.IllegalArgumentException: Invalid range: [104:0..103:1)

    at com.google.common.collect.Range.<init>(Range.java:334)
    at com.google.common.collect.Range.create(Range.java:134)
    at com.google.common.collect.Range.closedOpen(Range.java:171)
    at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.getNumberOfEntries(ManagedCursorImpl.java:1253)
    at org.apache.bookkeeper.mledger.impl.ManagedLedgerTest.testNeverThrowsMarkDeletingMarkedPositionInMaybeUpdateCursorBeforeTrimmingConsumedLedger(ManagedLedgerTest.java:3838)

3. 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.

4. Add testNeverThrowsMarkDeletingMarkedPositionInMaybeUpdateCursorBeforeTrimmingConsumedLedger test in ManagedLedgerImplTest to verify the code change.

5. Fix tests due to this PR's code change.

6. Fix some flaky tests introduced by PR [fix][broker] Fix cursor position persistence in ledger trimming #25087.

Verifying this change

• Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: oneby-wang#18

[github-actions]

@oneby-wang Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

[codecov-commenter]

Codecov Report

❌ Patch coverage is 84.21053% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.53%. Comparing base (bf98773) to head (3f88653).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	84.00%	2 Missing and 2 partials ⚠️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	88.00%	2 Missing and 1 partial ⚠️
...kkeeper/mledger/impl/ManagedLedgerFactoryImpl.java	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #25110       +/-   ##
=============================================
+ Coverage     38.39%   74.53%   +36.13%     
- Complexity    13035    34073    +21038     
=============================================
  Files          1842     1899       +57     
  Lines        145553   149737     +4184     
  Branches      16919    17410      +491     
=============================================
+ Hits          55890   111599    +55709     
+ Misses        81980    29252    -52728     
- Partials       7683     8886     +1203     

Flag	Coverage Δ
inttests	26.36% <68.42%> (-0.06%)	⬇️
systests	23.03% <66.66%> (-0.01%)	⬇️
unittests	74.05% <84.21%> (+39.86%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../bookkeeper/mledger/impl/NonDurableCursorImpl.java	82.45% <100.00%> (+3.88%)	⬆️
...kkeeper/mledger/impl/ManagedLedgerFactoryImpl.java	58.91% <60.00%> (+24.73%)	⬆️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	81.22% <88.00%> (+30.26%)	⬆️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	78.64% <84.00%> (+29.31%)	⬆️

... and 1408 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[oneby-wang]

/pulsarbot rerun-failure-checks

[oneby-wang]

Doesn't solve ManagedLedgerTest.testTrimmerRaceCondition() flaky test yet, I'll revisit it later.

Seems we must ensure some happens-before order between ManagedCursoImplr.asyncMarkDelete() and ManagedLedgerImpl.maybeUpdateCursorBeforeTrimmingConsumedLedger() after #25087.

[oneby-wang]

Fix ManagedLedgerTest.testTrimmerRaceCondition() flaky test.

Add ManagedLedgerTest.testTrimmerRaceConditionInNonDurableCursor() and ManagedLedgerTest.testTrimmerRaceConditionWithThrottleMarkDeleteInDurableCursor() test.

Ready for review.