[fix] [ml] fix discontinuous ledger deletion #20898

poorbarcode · 2023-07-28T10:29:22Z

Motivation

The task trim ledgers runs in the thread BkMainThreadPool.choose(ledgerName)
The task write entries to BK runs in the thread BkMainThreadPool.choose(ledgerId)

So the two tasks above may run concurrently/

The task trim ledgers work as the flow below:

find the ledgers which are no longer to read, the result is {Ledgers before the slowest read}.
check if the {Ledgers before the slowest read} is out of retention policy, the result is {Ledgers to be deleted}.
- if the create time of the ledger is lower than the earliest retention time, mark it should be deleted
- if after deleting this ledger, the rest ledgers are still larger than the retention size, mark it should be deleted
delete the{Ledgers to be deleted}

In step-3-2, the implementation is like this:

long totalSizeToDelete = 0;
for (Ledger ledger : ledegrs) {
    totalSizeToDelete += ledger.size;
    if (calculateTotalSizeWrited() - totalSizeToDelete >= sizeShouldRetention ){
       // mark the ledger should be deleted.
    }
}

see: https://github.com/apache/pulsar/blob/branch-3.0/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L2671-L2698

(Highlight) There is a scenario that causes the task trim ledgers did discontinuous ledger deletion, resulting consume messages discontinuous:

context:
- ledgers: [{id=1, size=100}, {id=2,size=100}]
- retention size: 150
- no cursor there
Check ledger 1, skip by retention check (200 - 100) < 150
One in-flight writing is finished, the calculateTotalSizeWrited() would return 300 now.
Check ledger 2, retention check (300 - 100) > 150, mark the ledger-2 should be deleted.
Delete the ledger 2.
Create a new consumer. It will receive messages from [ledger-1, ledegr-3], but the ledger-2 will be skipped.

Modifications

once the retention constraint has been met, break the loop.

Documentation

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

Matching PR in forked repository

PR in forked repository: x

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

…of the loop

…l/ManagedLedgerImpl.java Co-authored-by: Qiang Zhao <mattisonchao@apache.org>

mattisonchao

LGTM :)

mattisonchao · 2023-07-28T11:38:31Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

            }

            long slowestReaderLedgerId = -1;
+            final long slowestNonDurationLedgerId = getTheSlowestNonDurationReadPosition().getLedgerId();


We can introduce a new API to the cursor container to get this position and explain cursors.getSlowestReaderPosition() only can get the nonDurable cursor in the future by #17273. :)

Yes, we can improve it in the future

lifepuzzlefun

Nice catch !

coderzc · 2023-07-31T09:53:04Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

+                LedgerInfo ls = ledgerInfoIterator.next();
+                releaseReadHandleIfNoLongerRead(ls.getLedgerId(), slowestNonDurationLedgerId.getValue());
+            }
+


Can we break out of loops early?

No, all the ledgers in the collection may no longer need to be read

Can we break out of loops early, if ledgerId >= slowestNonDurationLedgerId?

Good suggestion, fixed.

gaoran10 · 2023-08-01T02:43:19Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

        // Handle the -1 size limit as "infinite" size quota
-        return config.getRetentionSizeInMB() >= 0
-                && TOTAL_SIZE_UPDATER.get(this) - sizeToDelete >= config.getRetentionSizeInMB() * MegaByte;
+        return retentionSizeInMB >= 0 && TOTAL_SIZE_UPDATER.get(this) - sizeToDelete >= retentionSizeInMB * MegaByte;


Can we use an immutable value to instead of TOTAL_SIZE_UPDATER.get(this)?

@gaoran10

Sorry, I misunderstood this before. It was fixed now, could you take a look again?

Technoboy-

LGTM

gaoran10

LGTM

codecov-commenter · 2023-08-01T12:11:18Z

Codecov Report

❌ Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.09%. Comparing base (9be0b52) to head (c1a3400).
⚠️ Report is 1783 commits behind head on master.

Files with missing lines	Patch %	Lines
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	85.71%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20898      +/-   ##
============================================
- Coverage     73.23%   73.09%   -0.15%     
- Complexity     3762    32129   +28367     
============================================
  Files          1874     1875       +1     
  Lines        139377   139403      +26     
  Branches      15328    15328              
============================================
- Hits         102068   101890     -178     
- Misses        29297    29450     +153     
- Partials       8012     8063      +51

Flag	Coverage Δ
inttests	`24.12% <41.17%> (-0.27%)`	⬇️
systests	`25.16% <61.76%> (-0.12%)`	⬇️
unittests	`72.38% <88.23%> (-0.12%)`	⬇️

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

Files with missing lines	Coverage Δ
...g/apache/pulsar/common/util/LazyLoadableValue.java	`100.00% <100.00%> (ø)`
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	`81.32% <85.71%> (-0.48%)`	⬇️

... and 79 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.

- The task `trim ledgers` runs in the thread `BkMainThreadPool.choose(ledgerName)` - The task `write entries to BK` runs in the thread `BkMainThreadPool.choose(ledgerId)` So the two tasks above may run concurrently/ The task `trim ledgers` work as the flow below: - find the ledgers which are no longer to read, the result is `{Ledgers before the slowest read}`. - check if the `{Ledgers before the slowest read}` is out of retention policy, the result is `{Ledgers to be deleted}`. - if the create time of the ledger is lower than the earliest retention time, mark it should be deleted - if after deleting this ledger, the rest ledgers are still larger than the retention size, mark it should be deleted - delete the`{Ledgers to be deleted}` **(Highlight)** There is a scenario that causes the task `trim ledgers` did discontinuous ledger deletion, resulting consume messages discontinuous: - context: - ledgers: `[{id=1, size=100}, {id=2,size=100}]` - retention size: 150 - no cursor there - Check `ledger 1`, skip by retention check `(200 - 100) < 150` - One in-flight writing is finished, the `calculateTotalSizeWrited()` would return `300` now. - Check `ledger 2`, retention check `(300 - 100) > 150`, mark the ledger-2 should be deleted. - Delete the `ledger 2`. - Create a new consumer. It will receive messages from `[ledger-1, ledegr-3]`, but the `ledger-2` will be skipped. Once the retention constraint has been met, break the loop. (cherry picked from commit 782e91f)

### Motivation - The task `trim ledgers` runs in the thread `BkMainThreadPool.choose(ledgerName)` - The task `write entries to BK` runs in the thread `BkMainThreadPool.choose(ledgerId)` So the two tasks above may run concurrently/ The task `trim ledgers` work as the flow below: - find the ledgers which are no longer to read, the result is `{Ledgers before the slowest read}`. - check if the `{Ledgers before the slowest read}` is out of retention policy, the result is `{Ledgers to be deleted}`. - if the create time of the ledger is lower than the earliest retention time, mark it should be deleted - if after deleting this ledger, the rest ledgers are still larger than the retention size, mark it should be deleted - delete the`{Ledgers to be deleted}` **(Highlight)** There is a scenario that causes the task `trim ledgers` did discontinuous ledger deletion, resulting consume messages discontinuous: - context: - ledgers: `[{id=1, size=100}, {id=2,size=100}]` - retention size: 150 - no cursor there - Check `ledger 1`, skip by retention check `(200 - 100) < 150` - One in-flight writing is finished, the `calculateTotalSizeWrited()` would return `300` now. - Check `ledger 2`, retention check `(300 - 100) > 150`, mark the ledger-2 should be deleted. - Delete the `ledger 2`. - Create a new consumer. It will receive messages from `[ledger-1, ledegr-3]`, but the `ledger-2` will be skipped. ### Modifications Once the retention constraint has been met, break the loop. (cherry picked from commit 782e91f)

### Motivation - The task `trim ledgers` runs in the thread `BkMainThreadPool.choose(ledgerName)` - The task `write entries to BK` runs in the thread `BkMainThreadPool.choose(ledgerId)` So the two tasks above may run concurrently/ The task `trim ledgers` work as the flow below: - find the ledgers which are no longer to read, the result is `{Ledgers before the slowest read}`. - check if the `{Ledgers before the slowest read}` is out of retention policy, the result is `{Ledgers to be deleted}`. - if the create time of the ledger is lower than the earliest retention time, mark it should be deleted - if after deleting this ledger, the rest ledgers are still larger than the retention size, mark it should be deleted - delete the`{Ledgers to be deleted}` **(Highlight)** There is a scenario that causes the task `trim ledgers` did discontinuous ledger deletion, resulting consume messages discontinuous: - context: - ledgers: `[{id=1, size=100}, {id=2,size=100}]` - retention size: 150 - no cursor there - Check `ledger 1`, skip by retention check `(200 - 100) < 150` - One in-flight writing is finished, the `calculateTotalSizeWrited()` would return `300` now. - Check `ledger 2`, retention check `(300 - 100) > 150`, mark the ledger-2 should be deleted. - Delete the `ledger 2`. - Create a new consumer. It will receive messages from `[ledger-1, ledegr-3]`, but the `ledger-2` will be skipped. ### Modifications Once the retention constraint has been met, break the loop. (cherry picked from commit 782e91f) (cherry picked from commit b87c0fb)

[fix] [ml] fix discontinuous ledger deletion

7044a72

poorbarcode self-assigned this Jul 28, 2023

poorbarcode added release/3.0.2 release/2.11.3 release/2.10.6 labels Jul 28, 2023

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 28, 2023

poorbarcode added type/bug The PR fixed a bug or issue reported a bug and removed doc-not-needed Your PR changes do not impact docs labels Jul 28, 2023

poorbarcode added this to the 3.2.0 milestone Jul 28, 2023

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 28, 2023

poorbarcode requested review from Technoboy-, codelipenghui, gaoran10, hangc0276 and mattisonchao July 28, 2023 10:33

if (log.isDebugEnabled())

5450797

mattisonchao reviewed Jul 28, 2023

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java Outdated Show resolved Hide resolved

final the variable slowestNonDurationLedgerId

25f618d

poorbarcode requested a review from mattisonchao July 28, 2023 10:59

mattisonchao reviewed Jul 28, 2023

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java Outdated Show resolved Hide resolved

poorbarcode and others added 2 commits July 28, 2023 19:15

mv config.getRetentionSizeInMB and config.getRetentionTimeMillis out …

44eec9d

…of the loop

Update managed-ledger/src/main/java/org/apache/bookkeeper/mledger/imp…

062c34c

…l/ManagedLedgerImpl.java Co-authored-by: Qiang Zhao <mattisonchao@apache.org>

poorbarcode requested a review from mattisonchao July 28, 2023 11:16

mattisonchao approved these changes Jul 28, 2023

View reviewed changes

mattisonchao reviewed Jul 28, 2023

View reviewed changes

poorbarcode added 3 commits July 28, 2023 19:50

lazy calculate slowestNonDurationLedgerId

ea1621c

lazy calculate slowestNonDurationLedgerId

abae04c

lazy calculate slowestNonDurationLedgerId

3190851

lifepuzzlefun approved these changes Jul 30, 2023

View reviewed changes

coderzc reviewed Jul 31, 2023

View reviewed changes

gaoran10 reviewed Aug 1, 2023

View reviewed changes

use totalSize directlly

bb03c26

poorbarcode requested review from coderzc and gaoran10 August 1, 2023 02:55

Technoboy- approved these changes Aug 1, 2023

View reviewed changes

gaoran10 approved these changes Aug 1, 2023

View reviewed changes

poorbarcode added 2 commits August 1, 2023 17:06

address comment

38d9bc8

address comment: break the loop

c1a3400

coderzc approved these changes Aug 1, 2023

View reviewed changes

poorbarcode merged commit 782e91f into apache:master Aug 1, 2023

poorbarcode deleted the fix/skipped_trim_ledger branch August 1, 2023 14:38

poorbarcode added cherry-picked/branch-2.10 cherry-picked/branch-2.11 cherry-picked/branch-3.0 labels Aug 1, 2023

[fix] [ml] fix discontinuous ledger deletion #20898

[fix] [ml] fix discontinuous ledger deletion #20898

Uh oh!

Conversation

poorbarcode commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

Uh oh!

Uh oh!

mattisonchao left a comment

Choose a reason for hiding this comment

Uh oh!

mattisonchao Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poorbarcode Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifepuzzlefun left a comment

Choose a reason for hiding this comment

Uh oh!

coderzc Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

coderzc Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

gaoran10 Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

Technoboy- left a comment

Choose a reason for hiding this comment

Uh oh!

gaoran10 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

poorbarcode commented Jul 28, 2023 •

edited

Loading

mattisonchao Jul 28, 2023 •

edited

Loading

poorbarcode Jul 28, 2023 •

edited

Loading

codecov-commenter commented Aug 1, 2023 •

edited

Loading