You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/11 11:54:56 UTC
[GitHub] [pulsar] wuzhanpeng opened a new pull request #11640: [Enhancement] avoid duplicate deletion of schema
wuzhanpeng opened a new pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640
### Motivation
Currently when I need to delete a namespace forcedly, the `NamespacesBase#internalDeleteNamespaceForcefully` will firstly list all the topics under the namespace and then delete them concurrently without distinguishing wheather the topic is partitioned or non-partitioned. This behaivor will cause duplicate deletion of schema when the topic list contains partitioned topics, because all the partitions of the partitioned topic share the same schema.
### Modifications
Distinguish partitioned topic to avoid duplicate deletion of the same schema.
### 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
This PR does not need to update the docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#discussion_r691802592
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
##########
@@ -415,14 +415,40 @@ protected void internalDeleteNamespaceForcefully(AsyncResponse asyncResponse, bo
try {
// firstly remove all topics including system topics
if (!topics.isEmpty()) {
+ Set<String> partitionedTopics = new HashSet<>();
+ Set<String> nonPartitionedTopics = new HashSet<>();
+
for (String topic : topics) {
try {
- futures.add(pulsar().getAdminClient().topics().deleteAsync(topic, true, true));
+ TopicName topicName = TopicName.get(topic);
+ if (topicName.isPartitioned()) {
+ String partitionedTopic = topicName.getPartitionedTopicName();
+ if (!partitionedTopics.contains(partitionedTopic)) {
+ // Distinguish partitioned topic to avoid duplicate deletion of the same schema
+ futures.add(pulsar().getAdminClient().topics().deletePartitionedTopicAsync(
+ partitionedTopic, true, true));
+ partitionedTopics.add(partitionedTopic);
+ }
+ } else {
+ futures.add(pulsar().getAdminClient().topics().deleteAsync(
+ topic, true, true));
+ nonPartitionedTopics.add(topic);
+ }
} catch (Exception e) {
- log.error("[{}] Failed to force delete topic {}", clientAppId(), topic, e);
- asyncResponse.resume(new RestException(e));
+ String errorMessage = String.format("Failed to force delete topic %s, "
+ + "but the previous deletion command of partitioned-topics:%s "
+ + "and non-partitioned-topics:%s have been sent out asynchronously. "
+ + "Reason: %s",
+ topic, partitionedTopics, nonPartitionedTopics, e.getCause());
+ log.error("[{}] {}", clientAppId(), errorMessage, e);
+ asyncResponse.resume(new RestException(Status.INTERNAL_SERVER_ERROR, errorMessage));
+ return;
}
}
+
+ log.info("Successfully send deletion command of partitioned-topics:{} "
Review comment:
It should be a debug level log? If we have many partitioned topics and non-partitioned-topics, we will get a very long log here, maybe print the topic count is better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wuzhanpeng commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-899367078
@eolivelli @hangc0276 Sorry for the late. I supplemented a test case. PTAL~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wuzhanpeng commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-899284986
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wuzhanpeng commented on a change in pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on a change in pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#discussion_r689395561
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
##########
@@ -1472,6 +1474,60 @@ public void testForceDeleteNamespace() throws Exception {
}
}
+ @Test
+ public void testDistinguishTopicTypeWhenForceDeleteNamespace() throws Exception {
+ conf.setForceDeleteNamespaceAllowed(true);
+ final String ns = "prop-xyz/distinguish-topic-type-ns";
+ final String exNs = "prop-xyz/ex-distinguish-topic-type-ns";
+ admin.namespaces().createNamespace(ns, 2);
+ admin.namespaces().createNamespace(exNs, 2);
+
+ final String p1 = "persistent://" + ns + "/p1";
+ final String p5 = "persistent://" + ns + "/p5";
+ final String np = "persistent://" + ns + "/np";
+
+ admin.topics().createPartitionedTopic(p1, 1);
+ admin.topics().createPartitionedTopic(p5, 5);
+ admin.topics().createNonPartitionedTopic(np);
+
+ final String exNp = "persistent://" + exNs + "/np";
+ admin.topics().createNonPartitionedTopic(exNp);
+ // insert an invalid topic name
+ pulsar.getLocalMetadataStore().put(
Review comment:
There are actually three scenarios I tested for partitioned topics, non-partitioned topics and exceptions while handling internal deletion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wuzhanpeng commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-898792201
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wuzhanpeng commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-899342199
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wuzhanpeng commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-897528300
> Good work
> Can you add a test case?
Sorry for the late reply! I will add a test later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#discussion_r689390887
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
##########
@@ -1472,6 +1474,60 @@ public void testForceDeleteNamespace() throws Exception {
}
}
+ @Test
+ public void testDistinguishTopicTypeWhenForceDeleteNamespace() throws Exception {
+ conf.setForceDeleteNamespaceAllowed(true);
+ final String ns = "prop-xyz/distinguish-topic-type-ns";
+ final String exNs = "prop-xyz/ex-distinguish-topic-type-ns";
+ admin.namespaces().createNamespace(ns, 2);
+ admin.namespaces().createNamespace(exNs, 2);
+
+ final String p1 = "persistent://" + ns + "/p1";
+ final String p5 = "persistent://" + ns + "/p5";
+ final String np = "persistent://" + ns + "/np";
+
+ admin.topics().createPartitionedTopic(p1, 1);
+ admin.topics().createPartitionedTopic(p5, 5);
+ admin.topics().createNonPartitionedTopic(np);
+
+ final String exNp = "persistent://" + exNs + "/np";
+ admin.topics().createNonPartitionedTopic(exNp);
+ // insert an invalid topic name
+ pulsar.getLocalMetadataStore().put(
Review comment:
why this is related to this patch ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-897249600
@hangc0276 thanks for triaging PR and issues. When you label PRs, if you find some PRs do not need to update doc, could you please help label the PR with `no-need-doc` in the future? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wuzhanpeng commented on a change in pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on a change in pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#discussion_r692691233
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
##########
@@ -415,14 +415,40 @@ protected void internalDeleteNamespaceForcefully(AsyncResponse asyncResponse, bo
try {
// firstly remove all topics including system topics
if (!topics.isEmpty()) {
+ Set<String> partitionedTopics = new HashSet<>();
+ Set<String> nonPartitionedTopics = new HashSet<>();
+
for (String topic : topics) {
try {
- futures.add(pulsar().getAdminClient().topics().deleteAsync(topic, true, true));
+ TopicName topicName = TopicName.get(topic);
+ if (topicName.isPartitioned()) {
+ String partitionedTopic = topicName.getPartitionedTopicName();
+ if (!partitionedTopics.contains(partitionedTopic)) {
+ // Distinguish partitioned topic to avoid duplicate deletion of the same schema
+ futures.add(pulsar().getAdminClient().topics().deletePartitionedTopicAsync(
+ partitionedTopic, true, true));
+ partitionedTopics.add(partitionedTopic);
+ }
+ } else {
+ futures.add(pulsar().getAdminClient().topics().deleteAsync(
+ topic, true, true));
+ nonPartitionedTopics.add(topic);
+ }
} catch (Exception e) {
- log.error("[{}] Failed to force delete topic {}", clientAppId(), topic, e);
- asyncResponse.resume(new RestException(e));
+ String errorMessage = String.format("Failed to force delete topic %s, "
+ + "but the previous deletion command of partitioned-topics:%s "
+ + "and non-partitioned-topics:%s have been sent out asynchronously. "
+ + "Reason: %s",
+ topic, partitionedTopics, nonPartitionedTopics, e.getCause());
+ log.error("[{}] {}", clientAppId(), errorMessage, e);
+ asyncResponse.resume(new RestException(Status.INTERNAL_SERVER_ERROR, errorMessage));
+ return;
}
}
+
+ log.info("Successfully send deletion command of partitioned-topics:{} "
Review comment:
Thanks for your suggestion! I have addressed the comment. PTAL~ @codelipenghui
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wuzhanpeng commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-898216173
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-897306318
> @hangc0276 thanks for triaging PR and issues. When you label PRs, if you find some PRs do not need to update doc, could you please help label the PR with `no-need-doc` in the future? Thanks
@Anonymitaet Ok, I will add doc label in the following PRs, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-896770197
I believe that this is a good idea.
Can you please add a test case ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on pull request #11640: [Enhancement] avoid duplicate deletion of schema
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11640:
URL: https://github.com/apache/pulsar/pull/11640#issuecomment-896914140
Nice catch!
Would you please add the test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org