support truncate topic #10326

jangwind · 2021-04-22T13:33:15Z

Motivation

Add support for truncate all data of the topic without disconnect the producers and consumers.

Modifications

Add a API to truncate data of the topic which can delete all inactive ledgers even if the data retention is enabled. The truncate operation will move all cursors to the end of the topic and delete all inactive ledgers. The truncate operation will not rollover the current ledger, so the active ledger will not been deleted. So for this cases, if users want to delete all data from the topic and currently no new data writes to the topic, all the data will be deleted after the ledger rollover triggered.

And, in the future we can add a separate command for rollover the ledger manually. Which can decouple with the truncate api. So that from the admin side, we can combine the rollover API and the truncate API to delete both the current active ledger and inactive ledgers.

Verifying this change

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: (yes)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (yes)
The admin cli options: (yes)
Anything that affects deployment: (no)

Documentation

Does this pull request introduce a new feature? (yes)

codelipenghui · 2021-04-22T13:59:18Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java

-import org.apache.pulsar.common.policies.data.Policies;
-import org.apache.pulsar.common.policies.data.TenantInfo;
-import org.apache.pulsar.common.policies.data.TopicStats;
+import org.apache.pulsar.common.policies.data.*;


Please avoid use start import

You're right. I've modified the configuration of IDE, and I'll fix these problems right away

codelipenghui · 2021-04-22T13:59:52Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java

 import com.google.common.collect.Range;
-import com.google.common.collect.Sets;
+import com.google.common.collect.*;


Please avoid use start import

You're right. I've modified the configuration of IDE, and I'll fix these problems right away

codelipenghui · 2021-04-22T14:02:06Z

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

@@ -3922,4 +3922,66 @@ protected void internalHandleResult(AsyncResponse asyncResponse,
            }
        }
    }
+
+    protected void internalTruncateTopic(AsyncResponse asyncResponse) {
+


We should check the topic ownership first and then get the topic reference. So that we can use the ManagedLedger of the topic to truncate directly.

I see. Thank you very much. I will modify them in the way of check the topic ownership first and then get the topic reference. At the same time, The implementation is moved from brokerService to Topic

codelipenghui

BTW, the truncate operation only can works on the persistent topic, so we'd better to add a check for the topic domain, we should return code 412 when the input topic name is a non-persistent topic.

add non-persistent funcation to throw exception by "nosupport truncate".

update partition topic assert.

…597-3

eolivelli

This is a great feature.
Please address @codelipenghui comments

eolivelli · 2021-04-23T12:00:01Z

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

+    @ApiOperation(value = "Truncate a topic.",
+            notes = "NonPersistentTopic is not support truncate.")
+    @ApiResponses(value = {
+            @ApiResponse(code = 412, message = "NonPersistentTopic is not support truncate")


NonPersistentTopic does not support truncate

Thank you very much, I will fix it as soon as possible.

eolivelli · 2021-04-23T12:00:46Z

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

+    @DELETE
+    @Path("/{tenant}/{namespace}/{topic}/truncate")
+    @ApiOperation(value = "Truncate a topic.",
+            notes = "The latest ledger cannot be deleted ,and only delete acknowledged ledgers.")


The latest ledger will not be deleted, and also only delete acknowledged ledgers can be deleted.

Thank you very much, I will fix it as soon as possible.

eolivelli · 2021-04-23T12:01:12Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java

-            .enableBatching(false)
-            .messageRoutingMode(MessageRoutingMode.SinglePartition)
-            .create();
+                .topic(topicName)


please do not reformat

eolivelli · 2021-04-23T12:01:55Z

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdPersistentTopics.java

@@ -286,6 +287,19 @@ void run() throws PulsarAdminException {
        }
    }

+    @Parameters(commandDescription = "Truncate a topic. \n"
+            + "\t\tThe topic will be truncate, but the latest ledger cannot be deleted.")


"The topic will be truncated, but the latest ledger cannot be deleted"

Thank you very much, I will fix it as soon as possible.

eolivelli · 2021-04-23T12:02:11Z

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java

@@ -484,6 +485,19 @@ void run() throws PulsarAdminException {
        }
    }

+    @Parameters(commandDescription = "Truncate a topic. \n"
+            + "\t\tThe topic will be truncate, but the latest ledger cannot be deleted.")


"The topic will be truncated, but the latest ledger cannot be deleted"

Thank you very much, I will fix it as soon as possible.

eolivelli · 2021-04-23T12:02:42Z

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

@@ -3922,4 +3922,66 @@ protected void internalHandleResult(AsyncResponse asyncResponse,
            }
        }
    }
+
+    protected void internalTruncateTopic(AsyncResponse asyncResponse) {
+


…ix testing move skipAllMessage

gaoran10

Great work! Left some comments.

We could add an entire test for this case, for example, create producer and consumer, produce and consume some messages, then execute the truncate API to check the result.

gaoran10 · 2021-04-25T05:36:24Z

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

@@ -18,6 +18,7 @@
 */
 package org.apache.bookkeeper.mledger;

+import java.util.concurrent.CompletableFuture;


Is this import necessary?

Sorry, I added this dependency in the previous modification, I will delete this dependency.

gaoran10 · 2021-04-25T07:25:38Z

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

+            Topic topic;
+            try {
+                validateAdminAccessForTenant(topicName.getTenant());
+                validateTopicOwnership(topicName, authoritative);
+                topic = getTopicReference(topicName);
+            } catch (Exception e) {
+                log.error("[{}] Failed to truncate topic {}", clientAppId(), topicName, e);
+                resumeAsyncResponseExceptionally(asyncResponse, e);
+                return;
+            }
+            CompletableFuture<Void> future = topic.truncate();
+            future.thenAccept(a -> {
+                asyncResponse.resume(new RestException(Response.Status.NO_CONTENT.getStatusCode(),
+                        Response.Status.NO_CONTENT.getReasonPhrase()));
+            }).exceptionally(e -> {
+                asyncResponse.resume(e);
+                return null;
+            });


It seems that this block is repeated.

Thank you for your comment. This block is based on (topicName.isPartitioned()==true) and another is based on (meta.partitions == 0). I will merge them Into a function.

gaoran10 · 2021-04-25T07:26:10Z

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

+                    Topic topic;
+                    try {
+                        validateAdminAccessForTenant(topicName.getTenant());
+                        validateTopicOwnership(topicName, authoritative);
+                        topic = getTopicReference(topicName);
+                    } catch (Exception e) {
+                        log.error("[{}] Failed to truncate topic {}", clientAppId(), topicName, e);
+                        resumeAsyncResponseExceptionally(asyncResponse, e);
+                        return;
+                    }
+                    CompletableFuture<Void> future = topic.truncate();
+                    future.thenAccept(a -> {
+                        asyncResponse.resume(new RestException(Response.Status.NO_CONTENT.getStatusCode(),
+                                Response.Status.NO_CONTENT.getReasonPhrase()));
+                    }).exceptionally(e -> {
+                        asyncResponse.resume(e);
+                        return null;
+                    });


It seems that this block repeated with above.

Thank you for your comment. This block is based on (topicName.isPartitioned()==true) and another is based on (meta.partitions == 0). I will merge them Into a function.

gaoran10 · 2021-04-25T08:01:32Z

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

+
+    /**
+     * Truncate ledgers
+     * The latest ledger cannot be deleted ,and only delete acknowledged ledgers


Only delete ledgers before the mark delete position, right?

You are right. The description has been changed to ‘The truncate operation will move all cursors to the end of the topic and delete all inactive ledgers.’

gaoran10 · 2021-04-25T08:07:33Z

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

+        final List<CompletableFuture<Void>> futures = Lists.newArrayList();
+        for (ManagedCursor cursor : cursors) {
+            final CompletableFuture<Void> future = new CompletableFuture<>();
+            cursor.asyncClearBacklog(new AsyncCallbacks.ClearBacklogCallback() {


Why need to clear backlog?

If it doesn't clearly backLog, it will be trim instead of truncate. So I do it.

Clear backlog will lose unacknowledged messages?

The original request is to delete retained message, retained messages are acknowledged messages, messages in backlog are unacknowledged messages.
There are already dedicated commands to clear whole backlog or expire certain messages in backlog, for this command we should just delete as much as retained message(messages already consumed and acknowledge by all consumers) so we shouldn't clear backlog here.

Thank you very much for your comments. I think ‘truncate' is more like deleting all the messages than deleting the consumed messages. And I want to add a 'trim' function to the next PR.

…597-3

tuteng · 2021-04-26T02:17:53Z

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

@@ -2316,10 +2329,10 @@ void internalTrimConsumedLedgers(CompletableFuture<?> promise) {
                    log.debug("[{}] Ledger {} skipped for deletion as it is currently being written to", name,
                            ls.getLedgerId());
                    break;
-                } else if (expired) {
+                } else if (expired || isTruncate) {
                    log.debug("[{}] Ledger {} has expired, ts {}", name, ls.getLedgerId(), ls.getTimestamp());


When expired is false and isTruncate is true, it seems that the debug info needs to be updated.

+1 please update log info.

Thank you very much. I will fix this debug info.

tuteng · 2021-04-26T02:18:33Z

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

                    log.debug("[{}] Ledger {} has expired, ts {}", name, ls.getLedgerId(), ls.getTimestamp());
                    ledgersToDelete.add(ls);
-                } else if (overRetentionQuota) {
+                } else if (overRetentionQuota || isTruncate) {
                    log.debug("[{}] Ledger {} is over quota", name, ls.getLedgerId());


When overRetentionQuota is false and isTruncate is true, it seems that the debug info needs to be updated.

Thank you very much. I will fix this debug info.

MarvinCai

As Enrico suggested, please remove white spaces/ format changes, especially those indention changes. For new code it's fine to use a good format of your own, but for existing code if everyone just keep reformatting existing code in their commit it'll just create too much noise in the pr and make it hard to review.

MarvinCai · 2021-04-26T09:39:01Z

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

@@ -2316,10 +2329,10 @@ void internalTrimConsumedLedgers(CompletableFuture<?> promise) {
                    log.debug("[{}] Ledger {} skipped for deletion as it is currently being written to", name,
                            ls.getLedgerId());
                    break;
-                } else if (expired) {
+                } else if (expired || isTruncate) {
                    log.debug("[{}] Ledger {} has expired, ts {}", name, ls.getLedgerId(), ls.getTimestamp());


+1 please update log info.

MarvinCai · 2021-04-26T09:43:23Z

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

+        final List<CompletableFuture<Void>> futures = Lists.newArrayList();
+        for (ManagedCursor cursor : cursors) {
+            final CompletableFuture<Void> future = new CompletableFuture<>();
+            cursor.asyncClearBacklog(new AsyncCallbacks.ClearBacklogCallback() {


The original request is to delete retained message, retained messages are acknowledged messages, messages in backlog are unacknowledged messages.
There are already dedicated commands to clear whole backlog or expire certain messages in backlog, for this command we should just delete as much as retained message(messages already consumed and acknowledge by all consumers) so we shouldn't clear backlog here.

MarvinCai · 2021-04-26T09:50:07Z

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

@@ -2156,6 +2157,14 @@ private void scheduleDeferredTrimming(CompletableFuture<?> promise) {
        scheduledExecutor.schedule(safeRun(() -> trimConsumedLedgersInBackground(promise)), 100, TimeUnit.MILLISECONDS);


are we still using this method? if not can just remove it.

Sorry, does this method is ‘scheduledExecutor.schedule()’ or 'trimConsumedLedgersInBackground'? If it's one of them, please tell me how to modify it.

I reviewed the code. This function is private and unused in the ManageLedgerImpl Class. So It should be removed

MarvinCai · 2021-05-10T15:47:15Z

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

+        CompletableFuture<Void> future = ledger2.asyncTruncate();
+        future.get();
+
+        assertTrue(ledger2.getLedgersInfoAsList().size() <= 1);


can we use setMaxEntriesPerLedger to make managedLedger create new ledger so even current ledger won't be deleted, we can see older ledgers get removed?

@MarvinCai We have a min rollover time limitation, the min value we are able to set is 1min. So I think maybe setMaxEntriesPerLedger can't work here.

eolivelli

Lgtm

…597-3

MarvinCai

LGTM

jiangbin and others added 7 commits April 16, 2021 21:25

complete truncate and tests

328ab58

Merge remote-tracking branch 'apache/master' into jiangbin-9597-3

d1265be

remove truncateCallback, and develop Testing

2d248fd

change truncate implements, and add testing

48a3cea

fix code style

50af56b

fix testing

eb50ede

Merge branch 'master' into jiangbin-9597-3

bc0a536

codelipenghui assigned jangwind Apr 22, 2021

codelipenghui added the area/admin label Apr 22, 2021

codelipenghui added this to the 2.8.0 milestone Apr 22, 2021

codelipenghui reviewed Apr 22, 2021

View reviewed changes

jiangbin added 6 commits April 23, 2021 00:32

remove ManagedLedgerFactory#asyncTruncate

71b7447

update broker truncate implements, from brokerServer to persistentTopic.

e8f2f55

add non-persistent funcation to throw exception by "nosupport truncate".

fix code style: import *

a5bd7f8

add nonPersistent topic testing.

69e0168

update partition topic assert.

fix code style

47d9487

Merge remote-tracking branch 'origin/jiangbin-9597-3' into jiangbin-9…

e578616

…597-3

eolivelli requested changes Apr 23, 2021

View reviewed changes

jiangbin and others added 5 commits April 23, 2021 22:53

truncate operation add clear backlog

9a0e413

fix The partition topic requires a call from the admin client, and f…

586c623

…ix testing move skipAllMessage

fix doc

bdd7e52

fix import

ead3404

Merge branch 'master' into jiangbin-9597-3

3019e04

gaoran10 reviewed Apr 25, 2021

View reviewed changes

jiangbin added 3 commits April 25, 2021 20:34

modify description and remove unused import

a849b59

merge block repeated

3b16b52

Merge remote-tracking branch 'origin/jiangbin-9597-3' into jiangbin-9…

4bf2edb

…597-3

tuteng reviewed Apr 26, 2021

View reviewed changes

codelipenghui requested a review from eolivelli April 26, 2021 03:15

MarvinCai requested changes Apr 26, 2021

View reviewed changes

jiangbin and others added 4 commits April 28, 2021 21:42

fix code style

e4d1796

update debug info

c019f5c

move the unused function

f57a43c

Merge branch 'master' into jiangbin-9597-3

4dfd809

MarvinCai reviewed May 10, 2021

View reviewed changes

codelipenghui approved these changes May 11, 2021

View reviewed changes

eolivelli approved these changes May 11, 2021

View reviewed changes

jiangbin added 3 commits May 11, 2021 20:48

fix code style

f567407

Merge remote-tracking branch 'origin/jiangbin-9597-3' into jiangbin-9…

e8dd76c

…597-3

fix: async function modify to sync function

bd0589f

MarvinCai approved these changes May 12, 2021

View reviewed changes

codelipenghui merged commit 99ccd1d into apache:master May 12, 2021

codelipenghui mentioned this pull request Aug 3, 2021

Add admin command to truncate pulsar topics (partitions) #2736

Closed

eolivelli mentioned this pull request Oct 28, 2021

[FEATURE] Implement DeleteRecords API (KafkaStreams support) streamnative/kop#867

Closed

yuruguo mentioned this pull request Mar 15, 2022

Ensure the deletion consistency of topic and schema #14608

Merged

1 task

tisonkun mentioned this pull request Dec 10, 2022

[Admin CLI]Add admin rest endpoint/CLI to trim ledgers for a topic upon given position. #9929

Closed

		@@ -2156,6 +2157,14 @@ private void scheduleDeferredTrimming(CompletableFuture<?> promise) {
		scheduledExecutor.schedule(safeRun(() -> trimConsumedLedgersInBackground(promise)), 100, TimeUnit.MILLISECONDS);

support truncate topic #10326

support truncate topic #10326

Conversation

jangwind commented Apr 22, 2021 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

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

codelipenghui left a 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

gaoran10 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

MarvinCai Apr 26, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarvinCai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarvinCai Apr 26, 2021 • 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

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

MarvinCai left a comment

Choose a reason for hiding this comment

jangwind commented Apr 22, 2021 •

edited

Loading

MarvinCai Apr 26, 2021 •

edited

Loading

MarvinCai Apr 26, 2021 •

edited

Loading