[improve] [ml] Persist mark deleted ops to ZK if create cursor ledger was failed #20935

poorbarcode · 2023-08-06T17:41:26Z

Motivation

The progress Persist mark deleted position is like this:

persist to BK
if failed to persist to BK, try to persist to ZK

But in the current implementation: if the cursor ledger was created failed, Pulsar will not try to persist to ZK. It makes if the cursor ledger created fails, a lot of ack records can not be persisted, and we will get a lot of repeat consumption after the BK recover.

Modifications

Try to persist the mark deleted position to ZK if the cursor ledger was created failed

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/ManagedCursorImpl.java

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java

codelipenghui

The change looks good to me.
Only add some minor comments to consolidate the test.

codelipenghui · 2023-08-09T07:43:51Z

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

            }
        }, null);
    }

+    void persistPositionMetaStore(MarkDeleteEntry mdEntry, final VoidCallback callback) {


Suggested change

void persistPositionMetaStore(MarkDeleteEntry mdEntry, final VoidCallback callback) {

void persistPositionToMetaStore(MarkDeleteEntry mdEntry, final VoidCallback callback) {

@poorbarcode Please check here.

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java

hangc0276 · 2023-08-10T02:43:26Z

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

+        };
+
+        if (State.NoLedger.equals(STATE_UPDATER.get(this))) {
+            persistPositionMetaStore(mdEntry, cb);


If the BookKeeper cluster runs into read-only mode and millions of consumers keep acknowledging the mark-delete position to brokers, we will persist the mark-delete position to the meta store without any throttle policy, will it bring high pressure or risk for the meta store? especially when we use Zookeeper as the Pulsar cluster's meta store service.

If the BookKeeper cluster runs into read-only mode and millions of consumers keep acknowledging the mark-delete position to brokers, we will persist the mark-delete position to the meta store without any throttle policy, will it bring high pressure or risk for the meta store? especially when we use Zookeeper as the Pulsar cluster's meta store service.

Some users may want to set BK to read-only, and stop sending messages but consume messages.

However, pulsar does not support this yet, because even if only the message is consumed, there is still some BK write operations, here are some examples:

If a transaction is used when consuming a message, a transaction log is also written

Compaction tasks are still written to BK

Topic policies depend on BK write

PIP-192: New Pulsar Broker Load Balancer #16691

PIP-195: New bucket based delayed message tracker #16763

PIP178:Multiple snapshots for transaction buffer #16042

... and so on

If we want to support the above scenario "Make BK read-only and only consume messages", we should need a large design.

we will persist the mark-delete position to the meta store without any throttle policy

@hangc0276 @poorbarcode Actually, we have a throttling policy that can be configured by managedLedgerDefaultMarkDeleteRateLimit. For improving this case, it looks like we'd better have a separate one for the zookeeper. So that users can persist to the cursor ledger for each second but persist to zookeeper for every 30 seconds.

@hangc0276 @poorbarcode Actually, we have a throttling policy that can be configured by managedLedgerDefaultMarkDeleteRateLimit. For improving this case, it looks like we'd better have a separate one for the zookeeper. So that users can persist to the cursor ledger for each second but persist to zookeeper for every 30 seconds.

Good idea. Since this solution should change broker.conf and PersistencePolicies, maybe a PIP is needed.

In the current PR, I've simpler this behavior to the following: persist to ZK only hits these two scenarios

the cursor ledger is not null, persist to ZK after persisting to BK failed(the original behavior before this PR)

the cursor ledger is null, and the cursor.markDeletedPosition==managedLedger.LAC. Since the ledger can not be created, no entry can be written now, so the variable managedLedger.LAC will not increase anymore. For per subscription, persist to ZK will only be executed once.

And in the future, we submit a PIP to add a rater limiter to further improve the behavior.

@codelipenghui @hangc0276 What do you think abort?

Yes, it looks good to me. And users can also change managedLedgerDefaultMarkDeleteRateLimit to reduce the impact.

Don' we need to consider shouldPersistUnackRangesToLedger() here, as well? If individualDeletedMessages is too big, bigger than maxUnackedRangesToPersistInMetadataStore, shouldn't we persist it to BK, instead of MetadataStore?

codelipenghui · 2023-08-14T02:13:29Z

/pulsarbot run-failure-checks

codelipenghui · 2023-08-17T09:45:01Z

/pulsarbot run-failure-checks

hangc0276 · 2023-08-17T11:32:05Z

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

+        };
+
+        if (State.NoLedger.equals(STATE_UPDATER.get(this))) {
+            if (mdEntry.newPosition.equals(ledger.getLastConfirmedEntry())) {


Please update the compare logic as we discussed to cover the case that LAC's entryId is -1 and add a test to cover it

codelipenghui · 2023-08-28T14:20:07Z

/pulsarbot run-failure-checks

… was failed

poorbarcode · 2023-08-30T14:50:59Z

Rebase master

codecov-commenter · 2023-08-31T02:35:50Z

Codecov Report

Merging #20935 (c29f906) into master (dab5b2f) will increase coverage by 0.11%.
Report is 4 commits behind head on master.
The diff coverage is 86.84%.

@@             Coverage Diff              @@
##             master   #20935      +/-   ##
============================================
+ Coverage     73.03%   73.14%   +0.11%     
- Complexity    32363    32392      +29     
============================================
  Files          1887     1887              
  Lines        139860   139879      +19     
  Branches      15384    15389       +5     
============================================
+ Hits         102140   102316     +176     
+ Misses        29634    29477     -157     
  Partials       8086     8086

Flag	Coverage Δ
inttests	`24.08% <5.26%> (?)`
systests	`25.11% <5.26%> (+<0.01%)`	⬆️
unittests	`72.44% <86.84%> (+0.04%)`	⬆️

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

Files Changed	Coverage Δ
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	`81.22% <83.33%> (-0.08%)`	⬇️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	`79.23% <88.46%> (+0.25%)`	⬆️

... and 77 files with indirect coverage changes

… was failed (#20935) The progress Persist mark deleted position is like this: - persist to BK - If failed to persist to BK, try to persist to ZK But in the current implementation: if the cursor ledger was created failed, Pulsar will not try to persist to ZK. It makes if the cursor ledger created fails, a lot of ack records can not be persisted, and we will get a lot of repeat consumption after the BK recover. Modifications: Try to persist the mark deleted position to ZK if the cursor ledger was created failed (cherry picked from commit 843b830)

… was failed (#20935) The progress Persist mark deleted position is like this: - persist to BK - If failed to persist to BK, try to persist to ZK But in the current implementation: if the cursor ledger was created failed, Pulsar will not try to persist to ZK. It makes if the cursor ledger created fails, a lot of ack records can not be persisted, and we will get a lot of repeat consumption after the BK recover. Modifications: Try to persist the mark deleted position to ZK if the cursor ledger was created failed

poorbarcode self-assigned this Aug 6, 2023

poorbarcode added the type/bug The PR fixed a bug or issue reported a bug label Aug 6, 2023

poorbarcode added this to the 3.2.0 milestone Aug 6, 2023

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

poorbarcode added the release/3.0.2 label Aug 6, 2023

codelipenghui reviewed Aug 8, 2023

View reviewed changes

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

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java Show resolved Hide resolved

poorbarcode requested a review from codelipenghui August 8, 2023 13:47

codelipenghui approved these changes Aug 9, 2023

View reviewed changes

hangc0276 reviewed Aug 10, 2023

View reviewed changes

Technoboy- closed this Aug 14, 2023

Technoboy- reopened this Aug 14, 2023

poorbarcode force-pushed the improve/create_cursor_ledger_fail_then_persist_to_ZK branch from 8460bb0 to 7c7e879 Compare August 14, 2023 03:32

poorbarcode requested a review from hangc0276 August 14, 2023 04:00

hangc0276 reviewed Aug 17, 2023

View reviewed changes

codelipenghui added the category/functionality Some functions are not working such as get errors label Aug 21, 2023

poorbarcode force-pushed the improve/create_cursor_ledger_fail_then_persist_to_ZK branch from c6b4554 to ebb705a Compare August 22, 2023 05:04

poorbarcode requested a review from hangc0276 August 22, 2023 05:07

poorbarcode added 10 commits August 30, 2023 22:50

[improve] [ml] Persist mark deleted ops to ZK if create cursor ledger…

37c6f10

… was failed

-

09caa67

improve the log

5c79ca1

address comment

592d3f0

address comment

da822a0

improve the test

6587c08

rename method to persistPositionToMetaStore

185ae32

Persist to ZK only catch up once

f68cc0a

support the scenario that entry id is -1

8ef13a3

cleanup after the test

9461ef0

poorbarcode force-pushed the improve/create_cursor_ledger_fail_then_persist_to_ZK branch from 4dd88ef to 9461ef0 Compare August 30, 2023 14:50

fix test

c29f906

Technoboy- added the release/3.1.1 label Aug 31, 2023

Technoboy- approved these changes Aug 31, 2023

View reviewed changes

poorbarcode merged commit 843b830 into apache:master Aug 31, 2023

poorbarcode added the cherry-picked/branch-3.0 label Aug 31, 2023

Technoboy- added the cherry-picked/branch-3.1 label Sep 5, 2023

poorbarcode mentioned this pull request May 7, 2024

[fix] [ml] Mark delete stuck due to switching cursor ledger fails #22662

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve] [ml] Persist mark deleted ops to ZK if create cursor ledger was failed #20935

[improve] [ml] Persist mark deleted ops to ZK if create cursor ledger was failed #20935

poorbarcode commented Aug 6, 2023

codelipenghui left a comment

codelipenghui Aug 9, 2023

codelipenghui Aug 14, 2023

poorbarcode Aug 14, 2023

hangc0276 Aug 10, 2023

poorbarcode Aug 10, 2023 •

edited

Loading

codelipenghui Aug 14, 2023 •

edited

Loading

poorbarcode Aug 14, 2023

codelipenghui Aug 16, 2023

heesung-sn Nov 9, 2024

codelipenghui commented Aug 14, 2023

codelipenghui commented Aug 17, 2023

hangc0276 Aug 17, 2023

poorbarcode Aug 22, 2023

codelipenghui commented Aug 28, 2023

poorbarcode commented Aug 30, 2023

codecov-commenter commented Aug 31, 2023

	void persistPositionMetaStore(MarkDeleteEntry mdEntry, final VoidCallback callback) {
	void persistPositionToMetaStore(MarkDeleteEntry mdEntry, final VoidCallback callback) {

[improve] [ml] Persist mark deleted ops to ZK if create cursor ledger was failed #20935

[improve] [ml] Persist mark deleted ops to ZK if create cursor ledger was failed #20935

Conversation

poorbarcode commented Aug 6, 2023

Motivation

Modifications

Documentation

Matching PR in forked repository

codelipenghui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poorbarcode Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

codelipenghui Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codelipenghui commented Aug 14, 2023

codelipenghui commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codelipenghui commented Aug 28, 2023

poorbarcode commented Aug 30, 2023

codecov-commenter commented Aug 31, 2023

Codecov Report

poorbarcode Aug 10, 2023 •

edited

Loading

codelipenghui Aug 14, 2023 •

edited

Loading