[PIP-129] Introduce intermediate state for ledger deletion #13575

wuzhanpeng · 2021-12-30T10:23:20Z

Master Issue: #13526

Motivation

This pull request is the specific implementation plan of PIP-129

Modifications

Mark the deletable ledgers before actually delete them
Add the ability to check and track deletable ledgers

Most changes are concentrated on org.apache.bookkeeper.mledger.ManagedLedger and org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.

Verifying this change

Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as managed-ledger.

This PR is just a refactoring of the ledger deletion. The original logic semantics has not changed. I think the existing test cases of unit test and integration test are sufficient for test coverage.

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

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

no-need-doc

Only involves the improvement and reconstruction of internal logic.

wuzhanpeng · 2022-01-10T12:29:21Z

/pulsarbot run-failure-checks

wuzhanpeng · 2022-01-11T01:42:43Z

/pulsarbot run-failure-checks

wuzhanpeng · 2022-01-11T06:03:11Z

/pulsarbot run-failure-checks

wuzhanpeng · 2022-01-11T09:26:47Z

@codelipenghui @hangc0276 Could you help review this PR?

wuzhanpeng · 2022-01-21T01:33:27Z

/pulsarbot run-failure-checks

wuzhanpeng · 2022-01-21T08:32:24Z

/pulsarbot run-failure-checks

wuzhanpeng · 2022-01-25T03:54:50Z

/pulsarbot run-failure-checks

wuzhanpeng · 2022-01-25T06:31:18Z

/pulsarbot run-failure-checks

codelipenghui · 2022-01-26T05:01:31Z

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

+
+    /**
+     * Mark deletable ledgers for bookkeeper and offload storage.
+     *
+     * @param deletableLedgerIds
+     * @param deletableOffloadedLedgerIds
+     */
+    void markDeletableLedgers(Collection<Long> deletableLedgerIds, Collection<Long> deletableOffloadedLedgerIds);
+
+    /**
+     * Get all deletable ledgers.
+     *
+     * @return all the deletable ledgers of the managed-ledger
+     */
+    Set<Long> getAllDeletableLedgers();
+
+    /**
+     * Get all deletable offloaded ledgers.
+     *
+     * @return all the deletable offloaded ledgers of the managed-ledger
+     */
+    Set<Long> getAllDeletableOffloadedLedgers();
+
+    /**
+     * Check and remove all the deletable ledgers.
+     */
+    void removeAllDeletableLedgers();


Do we need to expose these methods to the ManagedLedger interface? Looks they are only used in the internal of the ManagedLedgerImpl.

hangc0276

Please add tests for this feature to protect the ledger deletion logic. @wuzhanpeng

hangc0276 · 2022-01-26T06:07:41Z

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

+
+    /**
+     * Mark deletable ledgers for bookkeeper and offload storage.
+     *
+     * @param deletableLedgerIds
+     * @param deletableOffloadedLedgerIds
+     */
+    void markDeletableLedgers(Collection<Long> deletableLedgerIds, Collection<Long> deletableOffloadedLedgerIds);
+
+    /**
+     * Get all deletable ledgers.
+     *
+     * @return all the deletable ledgers of the managed-ledger
+     */
+    Set<Long> getAllDeletableLedgers();
+
+    /**
+     * Get all deletable offloaded ledgers.
+     *
+     * @return all the deletable offloaded ledgers of the managed-ledger
+     */
+    Set<Long> getAllDeletableOffloadedLedgers();
+
+    /**
+     * Check and remove all the deletable ledgers.
+     */
+    void removeAllDeletableLedgers();


hangc0276 · 2022-01-26T06:07:42Z

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

+
+    /**
+     * Mark deletable ledgers for bookkeeper and offload storage.
+     *
+     * @param deletableLedgerIds
+     * @param deletableOffloadedLedgerIds
+     */
+    void markDeletableLedgers(Collection<Long> deletableLedgerIds, Collection<Long> deletableOffloadedLedgerIds);
+
+    /**
+     * Get all deletable ledgers.
+     *
+     * @return all the deletable ledgers of the managed-ledger
+     */
+    Set<Long> getAllDeletableLedgers();
+
+    /**
+     * Get all deletable offloaded ledgers.
+     *
+     * @return all the deletable offloaded ledgers of the managed-ledger
+     */
+    Set<Long> getAllDeletableOffloadedLedgers();
+
+    /**
+     * Check and remove all the deletable ledgers.
+     */
+    void removeAllDeletableLedgers();


hangc0276 · 2022-01-26T06:15:19Z

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

+
+            // Mark deletable offloaded ledgers
+            Set<Long> deletableOffloadedLedgers = ledgersToDelete.stream()
+                    .filter(ls -> ls.getOffloadContext().hasUidMsb())


You'd better check ls.getOffloadContext().isComplete instead of ls.getOffloadContext().hasUidMsb()

This is mainly implemented according to the original logic. See

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

Lines 2716 to 2729 in c18d645

private void asyncDeleteLedger(long ledgerId, LedgerInfo info) {

if (!info.getOffloadContext().getBookkeeperDeleted()) {

// only delete if it hasn't been previously deleted for offload

asyncDeleteLedger(ledgerId, DEFAULT_LEDGER_DELETE_RETRIES);

}

if (info.getOffloadContext().hasUidMsb()) {

UUID uuid = new UUID(info.getOffloadContext().getUidMsb(), info.getOffloadContext().getUidLsb());

cleanupOffloaded(ledgerId, uuid,

OffloadUtils.getOffloadDriverName(info, config.getLedgerOffloader().getOffloadDriverName()),

OffloadUtils.getOffloadDriverMetadata(info, config.getLedgerOffloader().getOffloadDriverMetadata()),

"Trimming");

}

}

hangc0276 · 2022-01-26T06:39:18Z

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

+
+            if (allFailedLedgers.isEmpty()) {
+                log.info("[{}] ledgers: {} and offloaded ledgers: {} are deleted successfully.",
+                        name, deletableLedgers, deletableOffloadedLedgers);


deletableLedgers and deletableOffloadedLedgers are Set and not override toString, So it will print the object address instead of the items.

Those collection objects that inherit java.util.AbstractCollection have the ability to return a string representation of this collection (java.util.AbstractCollection#toString).

hangc0276 · 2022-01-26T06:40:45Z

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

+            if (log.isDebugEnabled()) {
+                log.debug("[{}] Successfully delete bookkeeper ledgers: {} and offloaded ledgers: {}. "
+                                + "Failed to delete bookkeeper ledgers: {} and offloaded ledgers: {}", name,
+                        succeedDeletedLedgers, succeedDeletedOffloadedLedgers,


The same above.

hangc0276 · 2022-01-26T06:45:43Z

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

@@ -2547,21 +2582,15 @@ void internalTrimLedgers(boolean isTruncate, CompletableFuture<?> promise) {
            store.asyncUpdateLedgerIds(name, getManagedLedgerInfo(), ledgersStat, new MetaStoreCallback<Void>() {
                @Override
                public void operationComplete(Void result, Stat stat) {
+                    // perform actual deletion
+                    removeAllDeletableLedgers();


it will block at most 30s to wait all the ledger delete operation completed. However, this method is called by meta store callback thread, which will block the thread and lead to deadlock.

The reason for performming removeAllDeletableLedgers synchronously here is that two locks of trimmer and metadata need to be released after the actual deletion. If the execution is asynchronous, the update process of the property map cannot be protected.

We should get the metadata lock in the deleting steps. The ledger deletion may cost a lot of time which may block other metadata operations.

The operationComplete will be called by bookkeeper-ml-scheduler thread pool, If it executed by synchronously, it will block other operations. We can use another new thread pool the execute it.
For the property map problem, could we use another thread safe data structure the store it?

hangc0276 · 2022-01-26T06:50:47Z

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

@@ -146,6 +151,13 @@

    protected static final int AsyncOperationTimeoutSeconds = 30;

+    protected static final String DELETABLE_LEDGER_MARKER_KEYWORD = "pulsar.ml.deletable.ledgers";


We will append the prefix for each ledger, and store the property map into ZNode. However, if there are thousands of ledgers to delete, the property map will cost too many storage space especially into Znode, which limit by 5MB. The key point is the prefix will duplicate for each ledger. We'd better redesign the data structure of the properties storage.

Thanks for your question. IMO, the problem of over-size znode should theoretically be a discussion about whether ManagedLedgerInfo should be stored on zk. Or we can think of it this way, the ledger information stored in the property map actually comes from the LedgerInfoList structure, and this part of the data is also stored in a znode. Therefore, if the number of ledgers is large, the corresponding ml-znode must be huge. To avoid complicating matters, I think we can skip this part in this proposal, and start a new discussion thread on this problem.

For current implementation, we shouldn't introduce more overload for current managedLedger Znode storage. we'd better reduce the prefix of keyword or redesign the data structure for properties storage.

Maybe we keep the d_ledgers is enough? The pulsar.ml looks useless.

hangc0276 · 2022-01-26T06:58:13Z

@wuzhanpeng We'd better expose the ledger delete state into metrics.

wuzhanpeng · 2022-02-11T06:19:04Z

According to the review comments, I have made the following modifications:

removed the related ledger deletion methods from the managed-ledger interface
added related unit tests for two-stage ledger deletion
added monitoring metrics during ledger deletion

@codelipenghui @hangc0276 PTAL at your convenience~

hangc0276 · 2022-02-16T02:39:00Z

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

@@ -2547,21 +2582,15 @@ void internalTrimLedgers(boolean isTruncate, CompletableFuture<?> promise) {
            store.asyncUpdateLedgerIds(name, getManagedLedgerInfo(), ledgersStat, new MetaStoreCallback<Void>() {
                @Override
                public void operationComplete(Void result, Stat stat) {
+                    // perform actual deletion
+                    removeAllDeletableLedgers();


The operationComplete will be called by bookkeeper-ml-scheduler thread pool, If it executed by synchronously, it will block other operations. We can use another new thread pool the execute it.
For the property map problem, could we use another thread safe data structure the store it?

hangc0276 · 2022-02-16T02:40:22Z

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

@@ -146,6 +151,13 @@

    protected static final int AsyncOperationTimeoutSeconds = 30;

+    protected static final String DELETABLE_LEDGER_MARKER_KEYWORD = "pulsar.ml.deletable.ledgers";


For current implementation, we shouldn't introduce more overload for current managedLedger Znode storage. we'd better reduce the prefix of keyword or redesign the data structure for properties storage.

hangc0276 · 2022-02-16T02:41:07Z

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

+        // let current ledger close
+        ml.rollCurrentLedgerIfFull();
+        // let retention expire
+        Thread.sleep(1500);


Avoiding to use sleep, it will introduce flaky test.

zymap · 2022-02-16T01:49:14Z

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

            // Update metadata
+            // Mark deletable ledgers
+            Set<Long> deletableLedgers = Stream
+                    .concat(


Why do we need to concat the ledgersToDelete and offloadedLedgersToDelete? Looks like they may duplicate in the deletableLedgers set.

There may be ledgers in both ledgersToDelete and offloadedLedgersToDelete that need to be deleted. They do have the potential to have duplicate ledgers and that's why toSet is used.

zymap · 2022-02-16T01:53:42Z

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

+        asyncDeleteLedger(ledgerId, retry, new DeleteLedgerCallback() {
+            @Override
+            public void deleteLedgerComplete(Object ctx) {
+


Don't we need to handle the callback?

This is the default implementation, which is used to be compatible with the original logic.

zymap · 2022-02-16T02:22:25Z

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

@@ -2547,21 +2582,15 @@ void internalTrimLedgers(boolean isTruncate, CompletableFuture<?> promise) {
            store.asyncUpdateLedgerIds(name, getManagedLedgerInfo(), ledgersStat, new MetaStoreCallback<Void>() {
                @Override
                public void operationComplete(Void result, Stat stat) {
+                    // perform actual deletion
+                    removeAllDeletableLedgers();


We should get the metadata lock in the deleting steps. The ledger deletion may cost a lot of time which may block other metadata operations.

zymap · 2022-02-17T07:08:40Z

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

@@ -146,6 +151,13 @@

    protected static final int AsyncOperationTimeoutSeconds = 30;

+    protected static final String DELETABLE_LEDGER_MARKER_KEYWORD = "pulsar.ml.deletable.ledgers";


Maybe we keep the d_ledgers is enough? The pulsar.ml looks useless.

wuzhanpeng · 2022-02-22T04:03:52Z

I have made some updates based on the comments, PTAL~ @hangc0276 @zymap

wuzhanpeng · 2022-02-22T06:06:39Z

/pulsarbot run-failure-checks

github-actions · 2022-03-25T02:01:28Z

The pr had no activity for 30 days, mark with Stale label.

wolfstudy · 2022-04-28T03:12:29Z

What is the stats of the change? @wuzhanpeng

github-actions · 2022-05-29T02:16:07Z

The pr had no activity for 30 days, mark with Stale label.

tisonkun · 2022-12-11T04:00:42Z

Closed as stale and conflict. Please rebase and resubmit the patch if it's still relevant.

github-actions bot assigned wuzhanpeng Dec 30, 2021

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 30, 2021

wuzhanpeng force-pushed the pip129_mark_and_check_deleted_ledger branch 3 times, most recently from 4d16622 to 92da60a Compare January 10, 2022 02:41

wuzhanpeng force-pushed the pip129_mark_and_check_deleted_ledger branch from c28414a to ca0ddc6 Compare January 11, 2022 03:29

wuzhanpeng changed the title ~~[WIP][PIP-129] Introduce intermediate state for ledger deletion~~ [PIP-129] Introduce intermediate state for ledger deletion Jan 11, 2022

codelipenghui added this to the 2.10.0 milestone Jan 18, 2022

hangc0276 requested review from codelipenghui, congbobo184, merlimat, hangc0276, 315157973, eolivelli and zymap January 20, 2022 08:13

hangc0276 added area/broker type/feature The PR added a new feature or issue requested a new feature labels Jan 20, 2022

wuzhanpeng force-pushed the pip129_mark_and_check_deleted_ledger branch from ca0ddc6 to 0c1ddf1 Compare January 20, 2022 09:28

wuzhanpeng force-pushed the pip129_mark_and_check_deleted_ledger branch from a737aae to 24af2c1 Compare January 21, 2022 06:40

merlimat added the release/note-required label Jan 22, 2022

codelipenghui reviewed Jan 26, 2022

View reviewed changes

codelipenghui removed this from the 2.10.0 milestone Jan 26, 2022

hangc0276 requested changes Jan 26, 2022

View reviewed changes

wuzhanpeng force-pushed the pip129_mark_and_check_deleted_ledger branch from 24af2c1 to 8366b5b Compare February 11, 2022 04:11

hangc0276 reviewed Feb 16, 2022

View reviewed changes

zymap reviewed Feb 17, 2022

View reviewed changes

wuzhanpeng added 10 commits February 21, 2022 19:18

separate mark and delete phrase

1c3c8b8

advance the mark phrase

4b0a97b

add max retry mechanism

084227c

fix testcase

83d6cee

fix testcase

cac7cff

fix testcase

e94424b

fix checkstyle

87bc9ba

fix testcase

3f5971d

address comments

9b0e5b8

address comments

f75891e

wuzhanpeng force-pushed the pip129_mark_and_check_deleted_ledger branch from 8366b5b to f75891e Compare February 22, 2022 04:00

fix checkstyle

407238c

github-actions bot added the lifecycle/stale label Mar 25, 2022

github-actions bot removed the lifecycle/stale label Apr 29, 2022

github-actions bot added the Stale label May 29, 2022

codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022

tisonkun closed this Dec 11, 2022

	private void asyncDeleteLedger(long ledgerId, LedgerInfo info) {
	if (!info.getOffloadContext().getBookkeeperDeleted()) {
	// only delete if it hasn't been previously deleted for offload
	asyncDeleteLedger(ledgerId, DEFAULT_LEDGER_DELETE_RETRIES);
	}

	if (info.getOffloadContext().hasUidMsb()) {
	UUID uuid = new UUID(info.getOffloadContext().getUidMsb(), info.getOffloadContext().getUidLsb());
	cleanupOffloaded(ledgerId, uuid,
	OffloadUtils.getOffloadDriverName(info, config.getLedgerOffloader().getOffloadDriverName()),
	OffloadUtils.getOffloadDriverMetadata(info, config.getLedgerOffloader().getOffloadDriverMetadata()),
	"Trimming");
	}
	}

		@@ -146,6 +151,13 @@

		protected static final int AsyncOperationTimeoutSeconds = 30;

		protected static final String DELETABLE_LEDGER_MARKER_KEYWORD = "pulsar.ml.deletable.ledgers";

[PIP-129] Introduce intermediate state for ledger deletion #13575

[PIP-129] Introduce intermediate state for ledger deletion #13575

Conversation

wuzhanpeng commented Dec 30, 2021 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

wuzhanpeng commented Jan 10, 2022

wuzhanpeng commented Jan 11, 2022

wuzhanpeng commented Jan 11, 2022

wuzhanpeng commented Jan 11, 2022

wuzhanpeng commented Jan 21, 2022

wuzhanpeng commented Jan 21, 2022

wuzhanpeng commented Jan 25, 2022

wuzhanpeng commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangc0276 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

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

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

Choose a reason for hiding this comment

hangc0276 commented Jan 26, 2022

wuzhanpeng commented Feb 11, 2022

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

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

wuzhanpeng commented Feb 22, 2022

wuzhanpeng commented Feb 22, 2022

github-actions bot commented Mar 25, 2022

wolfstudy commented Apr 28, 2022

github-actions bot commented May 29, 2022

tisonkun commented Dec 11, 2022

wuzhanpeng commented Dec 30, 2021 •

edited

Loading