When delete a topic, delete the topic policy together. #11316
Conversation
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Outdated
Show resolved
Hide resolved
| @@ -2580,8 +2582,19 @@ private boolean isSystemTopic(String topic) { | |||
| } | |||
| } | |||
|
|
|||
| public CompletableFuture<Void> deleteTopicPolicies(TopicName topicName) { | |||
| if (!pulsar().getConfig().isTopicLevelPoliciesEnabled()) { | |||
| return new CompletableFuture<>(); | |||
There was a problem hiding this comment.
Do you want to return a completed future?
There was a problem hiding this comment.
I'm not sure return completed future is the best way, so followed the origin code style, return completed fusture. I will modify it.
There was a problem hiding this comment.
Finally, I maintain the complete fusture way. I think use complete future is more suitable for this method. Cound you give some advice.
There was a problem hiding this comment.
@horizonzy I think you need a completed CompletableFuture right?
There was a problem hiding this comment.
Maybe the new CompletableFuture should be completed. Such as CompletableFuture.completedFuture(null);
There was a problem hiding this comment.
sorry, I wasn't clear.
I was suggesting to return "CompletableFuture.completedFuture(null)"
There was a problem hiding this comment.
sorry, I wasn't clear.
I was suggesting to return "CompletableFuture.completedFuture(null)"
My fault, I misunderstand it. :)
There was a problem hiding this comment.
@horizonzy Could you please help add some unit tests for the change? You can refer to TopicPoliciesTest.
fine. |
Hi, the ut have completed, PTAL and give some advice :) |
| Awaitility.await().until(() -> pulsar.getTopicPoliciesService().getTopicPolicies(TopicName.get(topic)) != null); | ||
| assertNotNull(pulsar.getTopicPoliciesService().getTopicPolicies(TopicName.get(topic))); |
There was a problem hiding this comment.
Can be simplified to Awaitility.await().untilAssert()
| @@ -2395,4 +2395,31 @@ public void testTopicRetentionPolicySetInManagedLedgerConfig() throws Exception | |||
| }); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testPolicyIsDeleteTogether() throws Exception { | |||
There was a problem hiding this comment.
We should add a test for covering the topic auto-deletion case.
| @@ -2580,8 +2582,19 @@ private boolean isSystemTopic(String topic) { | |||
| } | |||
| } | |||
|
|
|||
| public CompletableFuture<Void> deleteTopicPolicies(TopicName topicName) { | |||
| if (!pulsar().getConfig().isTopicLevelPoliciesEnabled()) { | |||
| return new CompletableFuture<>(); | |||
There was a problem hiding this comment.
@horizonzy I think you need a completed CompletableFuture right?
| TopicName cloneTopicName = topicName; | ||
| if (topicName.isPartitioned()) { | ||
| cloneTopicName = TopicName.get(topicName.getPartitionedTopicName()); | ||
| } |
There was a problem hiding this comment.
The method topicName.getPartitionedTopicName() already handled is the topic is a partition or not. so we don't need to check here.
…ait().until 2. add ut for delete policy on topic auto-delete 3.code clean.
# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
...ker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
Show resolved
Hide resolved
|
@eolivelli Can you please review this PR? |
(cherry picked from commit 00f8e57)
Fixes #11308