Ensure the deletion consistency of topic and schema by yuruguo · Pull Request #14608 · apache/pulsar

yuruguo · 2022-03-08T15:30:02Z

Master Issue: #12795
Dev email: discuss and reply

Motivation

Deleting topic also deletes its schema.

Documentation

no-need-doc

nlu90

LGTM except one question.

nlu90 · 2022-03-14T21:02:23Z

-                        futures.add(deleteSchema().thenApply(schemaVersion -> null));
-                    }
+
+                    futures.add(deleteSchema().thenApply(schemaVersion -> null));


What happens to topics with multiple versions fo schema? Will all versions be cleaned?

Yes, will delete all the versions

Hmmm, it's a little confusing for using persistent schema storage for a non-persistent topic, after the broker restart and the producer/consumer will not connect to a non-persistent topic, the non-persistent topic is equivalent to being deleted, In some cases that users using a random name for non-persistent topic, after the application restarts, the topic name will be changes, looks like here will introduce a schema resource leak.

codelipenghui

I noticed there is a behavior change that might impact users' applications if the schema auto uploading is disabled.

Before this change, users can delete the topic without deleting schema, so that the application can connect to the newly created topic with the same topic name and existing schema, but after this change, not able to connect to the topic again.

Hmmm, before, users usually use the delete topic to truncate data of the topic. After #10326 introduced truncate topic, I think for now users should to use the truncate method to cleanup data of the topic. So there is no reason why the user still wants to use this topic, but also delete it.

We'd better clarify in the document.

codelipenghui · 2022-03-15T02:27:33Z

-                        futures.add(deleteSchema().thenApply(schemaVersion -> null));
-                    }
+
+                    futures.add(deleteSchema().thenApply(schemaVersion -> null));


Yes, will delete all the versions

codelipenghui · 2022-03-15T02:32:58Z

-                        futures.add(deleteSchema().thenApply(schemaVersion -> null));
-                    }
+
+                    futures.add(deleteSchema().thenApply(schemaVersion -> null));


Hmmm, it's a little confusing for using persistent schema storage for a non-persistent topic, after the broker restart and the producer/consumer will not connect to a non-persistent topic, the non-persistent topic is equivalent to being deleted, In some cases that users using a random name for non-persistent topic, after the application restarts, the topic name will be changes, looks like here will introduce a schema resource leak.

yuruguo · 2022-03-15T12:02:20Z

I noticed there is a behavior change that might impact users' applications if the schema auto uploading is disabled.

Before this change, users can delete the topic without deleting schema, so that the application can connect to the newly created topic with the same topic name and existing schema, but after this change, not able to connect to the topic again.

Hmmm, before, users usually use the delete topic to truncate data of the topic. After #10326 introduced truncate topic, I think for now users should to use the truncate method to cleanup data of the topic. So there is no reason why the user still wants to use this topic, but also delete it.

We'd better clarify in the document.

I have added explanation in the CLI / API.
PTAL again, thx!

codelipenghui

Please also help add a test to make sure the schema been removed after the topic has been deleted.

codelipenghui · 2022-03-21T08:02:01Z

-                "--deleteSchema" }, description = "Delete schema while deleting topic")
+        @Parameter(names = {"-d", "--deleteSchema"}, description = "Delete schema while deleting topic, "
+                + "but the parameter is invalid and the schema is always deleted", hidden = true)
        private boolean deleteSchema = false;


Should also update the default value to true?

Maybe we can't change the default value to true because that would change its usage.
Before modification: it means false if you don't specify -d, and it means true if you specify -d;
After modification: it means true if you don’t specify -d, but it means false only you specify -d false.
The two are not fully compatible.
It is similar to #12128

codelipenghui · 2022-03-21T08:02:11Z

-                "--deleteSchema" }, description = "Delete schema while deleting topic")
+        @Parameter(names = {"-d", "--deleteSchema"}, description = "Delete schema while deleting topic, "
+                + "but the parameter is invalid and the schema is always deleted", hidden = true)
        private boolean deleteSchema = false;


Should also update the default value to true?

same with above

codelipenghui · 2022-03-21T08:09:07Z

-                            if (deleteSchema) {
-                                return pulsar().getBrokerService()
-                                        .deleteSchemaStorage(topicName.getPartition(0).toString())
-                                        .thenCompose(unused ->
-                                                internalRemovePartitionsAuthenticationPoliciesAsync(numPartitions))
-                                        .thenCompose(unused2 ->
-                                                internalRemovePartitionsTopicAsync(numPartitions, force));
-                            }


If remove these lines, how does the method remove the schema?

internalRemovePartitionsTopicAsync(numPartitions, force) will delete schema by default.

codelipenghui · 2022-03-21T08:10:42Z

-                return t.deleteSchema().thenCompose(schemaVersion -> {
-                    log.info("Successfully delete topic {}'s schema of version {}", t.getName(), schemaVersion);
-                    return t.delete();
-                });


If remove these lines how do we delete the schema?

t.delete() will delete schema by default.

details: delete() -> delete(boolean,boolean,boolean) -> deleteSchema()

yuruguo · 2022-03-23T19:20:45Z

Please also help add a test to make sure the schema been removed after the topic has been deleted.

The test testDeleteTopicAndSchemaForV1 in SchemaTest can cover this change for v1/topic and I have added addtional test for v2/topic.

* Ensure the deletion consistency of topic and schema (cherry picked from commit c06b17d)

…4608)" This reverts commit 87a3323.

github-actions Bot assigned yuruguo Mar 8, 2022

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Mar 8, 2022

Jason918 reviewed Mar 10, 2022

View reviewed changes

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

nlu90 reviewed Mar 14, 2022

View reviewed changes

codelipenghui added this to the 2.11.0 milestone Mar 15, 2022

codelipenghui added the release/important-notice The changes which are important should be mentioned in the release note label Mar 15, 2022

codelipenghui reviewed Mar 15, 2022

View reviewed changes

yuruguo requested review from Jason918 and codelipenghui March 18, 2022 05:29

codelipenghui requested changes Mar 21, 2022

View reviewed changes

codelipenghui requested review from 315157973 and hangc0276 March 21, 2022 08:11

yuruguo requested a review from codelipenghui March 28, 2022 23:29

codelipenghui approved these changes Apr 13, 2022

View reviewed changes

codelipenghui requested review from Technoboy- and gaoran10 April 13, 2022 04:27

yuruguo added 3 commits April 27, 2022 16:02

Ensure the deletion consistency of topic and schema

073a420

Address comment

b3e74de

Add test

9dedaef

yuruguo requested a review from eolivelli April 27, 2022 12:14

yuruguo merged commit c06b17d into apache:master Apr 27, 2022

yuruguo added the area/schema label Apr 27, 2022

poorbarcode mentioned this pull request Aug 12, 2022

[improve][admin] Unset namespace policy to improve deleting namespace. #17033

Merged

1 task

dlg99 pushed a commit to dlg99/pulsar that referenced this pull request Nov 8, 2022

Ensure the deletion consistency of topic and schema (apache#14608)

7c69c40

* Ensure the deletion consistency of topic and schema (cherry picked from commit c06b17d)

dlg99 pushed a commit to datastax/pulsar that referenced this pull request Nov 8, 2022

Ensure the deletion consistency of topic and schema (apache#14608)

87a3323

* Ensure the deletion consistency of topic and schema (cherry picked from commit c06b17d)

mattisonchao mentioned this pull request Nov 21, 2022

[fix][schema] Fix cherry-pick issue from #18283 #18555

Merged

5 tasks

nicoloboschi added a commit to datastax/pulsar that referenced this pull request Jan 10, 2023

Revert "Ensure the deletion consistency of topic and schema (apache#1…

6981f34

…4608)" This reverts commit 87a3323.

nicoloboschi added a commit to datastax/pulsar that referenced this pull request Jan 10, 2023

Revert "Ensure the deletion consistency of topic and schema (apache#1…

fa6553e

…4608)" This reverts commit 87a3323.

Uh oh!

Conversation

yuruguo commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Documentation

Uh oh!

Uh oh!

nlu90 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuruguo commented Mar 15, 2022

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuruguo Mar 23, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

yuruguo Mar 23, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

yuruguo Mar 23, 2022 • 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!

yuruguo commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yuruguo commented Mar 8, 2022 •

edited

Loading

yuruguo Mar 23, 2022 •

edited

Loading

yuruguo Mar 23, 2022 •

edited

Loading

yuruguo Mar 23, 2022 •

edited

Loading

yuruguo commented Mar 23, 2022 •

edited

Loading