[PIP-186] Introduce two phase deletion protocol based on system topic #16590

horizonzy · 2022-07-14T02:13:54Z

Original issue: #13238
PIP Issue: #16569

Motivation

In current ledger deletion, we divided it into two separate steps. It happens in ManagedLedger and ManagedCursor.
Remove all the waiting to delete ledgers from the ledger list and update the newest ledger list into a meta store.
In the meta store update callback operation, delete the waiting to delete ledgers from storage systems, such as BookKeeper or Tiered storage.

Due to the separate step, we can’t ensure the ledger deletion transaction. If the first step succeeds and the second step fails, it will lead to ledgers that can't be deleted from the storage system forever. The second step may fail by broker restart or storage system deletion failed.

In our customer’s environment, we have found many orphan ledgers cause by the above reason.

Modifications

Introduce two phase deletion to solve the problem.

Need to update docs?

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

…time.

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

eolivelli · 2022-08-16T15:16:52Z

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

+            //No op
+        }
+
+        private static final CompletableFuture<?> COMPLETABLE_FUTURE = CompletableFuture.completedFuture(null);


you cannot cache CompletableFutures
the behaviour will be unpredictable

eolivelli

good work
I have left some feedback

eolivelli · 2022-08-16T15:20:19Z

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

+                        ledgersStat = stat;
+                        metadataMutex.unlock();
+                        trimmerMutex.unlock();
+                        if (ledgerDeletionService instanceof LedgerDeletionService.LedgerDeletionServiceDisable) {


using instanceof is a bad practice.
we should add a method to LedgerDeletionService and override it on the various implementations

eolivelli · 2022-08-16T15:22:53Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java

+            @ApiResponse(code = 404, message = "Topic does not exist"),
+            @ApiResponse(code = 412, message = "Topic has active producers/subscriptions"),
+            @ApiResponse(code = 500, message = "Internal server error")})
+    public void deleteLedger(


why do we need a new API?

If there are 2 brokers, it will exists two system topic consumers(A and B). If the topic onwership is A, when B received pending delete ledger msg, it will send delete ledger command to A. So introduce rest api in server side and PusarAdmin.

# Conflicts: # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/offload/OffloadUtils.java

hangc0276 · 2022-09-05T03:49:55Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+            doc = "Using two phase deletion when delete ledger. if true, "
+                    + "LedgerDeletionService will take over ledger deletion. (Default false)"
+    )
+    private boolean topicTwoPhaseDeletionEnabled;


We should set the default value to false

hangc0276 · 2022-09-05T04:20:55Z

...er/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedLedgerDeletionService.java

+    }
+
+    @Override
+    public void close() throws Exception {


We need to stop the statsProvider on close

hangc0276 · 2022-09-05T04:26:16Z

...er/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedLedgerDeletionService.java

+        }
+        this.statsProvider.start(configuration);
+        StatsLogger statsLogger = statsProvider.getStatsLogger("pulsar_ledger_deletion");
+        this.deleteLedgerOpLogger = statsLogger.getOpStatsLogger("delete_ledger");


Does the metrics can be exported to the broker Prometheus port?

hangc0276 · 2022-09-05T05:54:06Z

...er/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedLedgerDeletionService.java

+                    future.complete(null);
+                });
+            } else if (LedgerComponent.MANAGED_CURSOR == pendingDeleteLedger.getLedgerComponent()) {
+                future.complete(null);


Please add @TODO flag here.

hangc0276 · 2022-09-05T07:23:14Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+                                        }
+                                        future.complete(null);
+                                    });
+                        } else if (LedgerType.OFFLOAD_LEDGER == ledgerType) {


Can we merge these two cases into one? The main differences are the offloadContext parameter. In managedledger.asyncDeleteLedger method, we also checked the ledgerType

Yes, it can be.

hangc0276 · 2022-09-05T07:26:15Z

...er/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedLedgerDeletionService.java

+                    reader.ackMessageAsync(message);
+                    future.complete(null);
+                } else if (ex instanceof PulsarAdminException.ServerSideErrorException) {
+


Do we need to deal with this exception?

It needn't, The origin logic is: in client side, to check the ServerSideErrorException if contains PendingDeleteLedgerInvalidException info, it contains it, means that the pending delete message is invalid, we also need ack it.
But now, at server side, we catch PendingDeleteLedgerInvalidException, make it return success response. So the client side needn't to check the error response info.

I will delete this logic branch.

hangc0276 · 2022-09-05T07:50:23Z

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

+            CompletableFuture<?> appendDeleteLedgerFuture =
+                    appendPendingDeleteLedger(deletableLedgers, deletableOffloadedLedgers);
+            appendDeleteLedgerFuture.thenAccept(ignore -> {
+                believedDeleteIds.addAll(deletableLedgers);


If the ledgerDeletionService not enabled, the believedDeleteIds set will keep increasing and doesn't have chance to delete items.

Nice catch.

# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

github-actions · 2022-10-06T02:17:31Z

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

# Conflicts: # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerFactory.java # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

horizonzy · 2022-10-18T03:20:47Z

@hangc0276 @eolivelli Could you help to review it, thanks! Pending for a long time.

github-actions · 2022-11-19T02:12:24Z

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

Anonymitaet · 2023-07-21T04:03:37Z

Confirmed w/ @codelipenghui, for 3.1, the code will be frozen next week, and this PR will not be merged into 3.1.

horizonzy added 30 commits May 28, 2022 14:10

init snip-145

0c1a891

redefine TrashDataComponent proto

df055a6

fix compile problem.

4731f73

fix code style.

e37e5c4

fix code format.

5d04e3d

complete ManagedTrashMXBean

b009d7e

fix trashData value null problem.

93ca968

fix EMPTY_LEDGER_INFO initialize problem.

a133007

remove triggerDelete schedule task.

c5ca979

code clean.

61a0e91

revert broker.conf

464a407

code format.

2a34693

optimize persist data when the context is empty.

3e571e2

complete admin api.

699d05a

support trigger config.

2f9acb2

fix check style

79deefd

compatible

510da88

support config to control is support two phase deletion.

9dd35c3

code style fix.

0f97c20

make ManagedTrash cover more case.

488d2ce

make triggerDeleteInBackground to interface.

234dd94

make triggerDeleteInBackground to interface.

cfb950d

fix ignore uncompleted offload ledger problem.

428bf11

when close managedLedger, close managedTrash together.

cb6e122

use object to take place of string on trashData key.

02c7867

change TrashDataKey -> TrashKey.

26e55a1

add backoff policy to avoid same key to delete frequently in a short …

5a2c4a4

…time.

code clean.

59a21fc

complete unit test.

c121e42

Merge branch 'master' into snip-145

7e6c007

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

eolivelli requested changes Aug 16, 2022

View reviewed changes

horizonzy added 6 commits August 17, 2022 10:04

address comment.

c7a9a3f

code clean.

b76a379

Merge branch 'master' into snip-48

c2fa18f

# Conflicts: # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/offload/OffloadUtils.java

fix checkstyle.

3b88e3c

fix checkstyle.

b3b6286

fix compatible problem.

ffafe97

hangc0276 reviewed Sep 5, 2022

View reviewed changes

horizonzy added 2 commits September 6, 2022 00:52

address comment.

51ebc0a

Merge branch 'master' into snip-48

1e0d9da

# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

github-actions bot added the Stale label Oct 6, 2022

github-actions bot removed the Stale label Oct 19, 2022

dlg99 mentioned this pull request Oct 26, 2022

[fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent. #18193

Merged

4 tasks

github-actions bot added the Stale label Nov 19, 2022

RobertIndie modified the milestones: 3.0.0, 3.1.0 Apr 11, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PIP-186] Introduce two phase deletion protocol based on system topic #16590

[PIP-186] Introduce two phase deletion protocol based on system topic #16590

horizonzy commented Jul 14, 2022 •

edited

Loading

eolivelli Aug 16, 2022

horizonzy Aug 16, 2022

eolivelli left a comment

eolivelli Aug 16, 2022

horizonzy Aug 16, 2022

eolivelli Aug 16, 2022

horizonzy Aug 16, 2022

hangc0276 Sep 5, 2022

horizonzy Sep 5, 2022

hangc0276 Sep 5, 2022

horizonzy Sep 5, 2022

hangc0276 Sep 5, 2022

hangc0276 Sep 5, 2022

horizonzy Sep 5, 2022

hangc0276 Sep 5, 2022

horizonzy Sep 5, 2022

hangc0276 Sep 5, 2022

horizonzy Sep 5, 2022 •

edited

Loading

hangc0276 Sep 5, 2022

horizonzy Sep 5, 2022

github-actions bot commented Oct 6, 2022

horizonzy commented Oct 18, 2022

github-actions bot commented Nov 19, 2022

Anonymitaet commented Jul 21, 2023

[PIP-186] Introduce two phase deletion protocol based on system topic #16590

Are you sure you want to change the base?

[PIP-186] Introduce two phase deletion protocol based on system topic #16590

Conversation

horizonzy commented Jul 14, 2022 • edited Loading

Motivation

Modifications

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli 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

horizonzy Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2022

horizonzy commented Oct 18, 2022

github-actions bot commented Nov 19, 2022

Anonymitaet commented Jul 21, 2023

horizonzy commented Jul 14, 2022 •

edited

Loading

horizonzy Sep 5, 2022 •

edited

Loading