[GitHub] [pulsar] codelipenghui commented on a change in pull request #11316: When delete a topic, delete the topic policy together.

[GitBox <gi...@apache.org>]

codelipenghui commented on a change in pull request #11316:
URL: https://github.com/apache/pulsar/pull/11316#discussion_r670389690



##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java
##########
@@ -2395,4 +2395,31 @@ public void testTopicRetentionPolicySetInManagedLedgerConfig() throws Exception
         });
     }
 
+    @Test
+    public void testPolicyIsDeleteTogether() throws Exception {

Review comment:
       We should add a test for covering the topic auto-deletion case.

##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java
##########
@@ -2395,4 +2395,31 @@ public void testTopicRetentionPolicySetInManagedLedgerConfig() throws Exception
         });
     }
 
+    @Test
+    public void testPolicyIsDeleteTogether() throws Exception {
+        final String topic = testTopic + UUID.randomUUID();
+        pulsarClient.newProducer().topic(topic).create().close();
+
+        Awaitility.await()
+                .until(() -> pulsar.getTopicPoliciesService().cacheIsInitialized(TopicName.get(topic)));
+        assertNull(pulsar.getTopicPoliciesService().getTopicPolicies(TopicName.get(topic)));
+
+        int maxConsumersPerSubscription = 10;
+        admin.topics().setMaxConsumersPerSubscription(topic, maxConsumersPerSubscription);
+
+        Awaitility.await().until(() -> pulsar.getTopicPoliciesService().getTopicPolicies(TopicName.get(topic)) != null);
+        assertNotNull(pulsar.getTopicPoliciesService().getTopicPolicies(TopicName.get(topic)));

Review comment:
       Can be simplified to Awaitility.await().untilAssert()

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -2580,6 +2581,17 @@ private boolean isSystemTopic(String topic) {
         }
     }
 
+    public CompletableFuture<Void> deleteTopicPolicies(TopicName topicName) {
+        if (!pulsar().getConfig().isTopicLevelPoliciesEnabled()) {
+            return new CompletableFuture<>();
+        }
+        TopicName cloneTopicName = topicName;
+        if (topicName.isPartitioned()) {
+            cloneTopicName = TopicName.get(topicName.getPartitionedTopicName());
+        }

Review comment:
       The method `topicName.getPartitionedTopicName()` already handled is the topic is a partition or not. so we don't need to check here.

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -2580,8 +2582,19 @@ private boolean isSystemTopic(String topic) {
         }
     }
 
+    public CompletableFuture<Void> deleteTopicPolicies(TopicName topicName) {
+        if (!pulsar().getConfig().isTopicLevelPoliciesEnabled()) {
+            return new CompletableFuture<>();

Review comment:
       @horizonzy I think you need a completed `CompletableFuture` right?




-- 
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