poorbarcode commented on code in PR #18307:
URL: https://github.com/apache/pulsar/pull/18307#discussion_r1013556251
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -1207,7 +1207,14 @@ private CompletableFuture<Void> delete(boolean
failIfHasSubscriptions,
brokerService.deleteTopicAuthenticationWithRetry(topic,
deleteTopicAuthenticationFuture, 5);
deleteTopicAuthenticationFuture.thenCompose(__ ->
deleteSchema())
- .thenCompose(__ -> deleteTopicPolicies())
+ .thenCompose(__ -> {
+ if
(!this.getBrokerService().getPulsar().getBrokerService()
Review Comment:
I think about it carefully, and now it's reasonable(even if other system
topics have policies). Because:
- Deleting `__change_event` deletes all the policies for the topic.
- Except for deleting the namespace, no other situation will delete
`__change_event`.
Should we add some code-comment and a test for this?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -218,38 +218,47 @@ protected CompletableFuture<Void>
internalDeleteNamespaceAsync(boolean force) {
}))
.thenCompose(topics -> {
List<String> allTopics = topics.get(0);
+ ArrayList<String> copyAllTopics = new ArrayList<>();
List<String> allPartitionedTopics = topics.get(1);
- if (!force) {
- boolean hasNonSystemTopic = false;
- for (String topic : allTopics) {
- if
(!pulsar().getBrokerService().isSystemTopic(TopicName.get(topic))) {
- hasNonSystemTopic = true;
- break;
- }
+ ArrayList<String> copyAllPartitionTopics = new
ArrayList<>();
+ boolean hasNonSystemTopic = false;
+ List<String> allSystemTopics = new ArrayList<>();
+ List<String> allPartitionedSystemTopics = new
ArrayList<>();
+ for (String topic : allTopics) {
+ if
(!pulsar().getBrokerService().isSystemTopic(TopicName.get(topic))) {
+ hasNonSystemTopic = true;
+ copyAllTopics.add(topic);
+ } else {
+ allSystemTopics.add(topic);
}
- if (!hasNonSystemTopic) {
- for (String topic : allPartitionedTopics) {
- if
(!pulsar().getBrokerService().isSystemTopic(TopicName.get(topic))) {
- hasNonSystemTopic = true;
- break;
- }
- }
+ }
+ for (String topic : allPartitionedTopics) {
+ if
(!pulsar().getBrokerService().isSystemTopic(TopicName.get(topic))) {
+ hasNonSystemTopic = true;
+ copyAllPartitionTopics.add(topic);
+ } else {
+ allPartitionedSystemTopics.add(topic);
}
-
+ }
+ if (!force) {
if (hasNonSystemTopic) {
throw new RestException(Status.CONFLICT, "Cannot
delete non empty namespace");
}
}
return
namespaceResources().setPoliciesAsync(namespaceName, old -> {
old.deleted = true;
return old;
- }).thenCompose(__ -> {
- return internalDeleteTopicsAsync(allTopics);
- }).thenCompose(__ -> {
- return
internalDeletePartitionedTopicsAsync(allPartitionedTopics);
+ }).thenCompose(ignore -> {
+ return internalDeleteTopicsAsync(copyAllTopics);
+ }).thenCompose(ignore -> {
+ return
internalDeletePartitionedTopicsAsync(copyAllPartitionTopics);
+ }).thenCompose(ignore -> {
+ return internalDeleteTopicsAsync(allSystemTopics);
+ }).thenCompose(ignore__ -> {
+ return
internalDeletePartitionedTopicsAsync(allPartitionedSystemTopics);
Review Comment:
We should add a separate test: Delete namespaces that contain both system
topic and non-system topic, preferably with topics ordered like this
`[__change_event, non-system-topic]`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
