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 2022/05/27 06:27:12 UTC

[GitHub] [pulsar] mattisonchao opened a new pull request, #15811: [fix][broker] Avoid using `thenAccept` invoke async method.

mattisonchao opened a new pull request, #15811:
URL: https://github.com/apache/pulsar/pull/15811

   ### Motivation
   
   Avoid invoking async methods with `thenAccept`. It will ignore the exception and do not waiting for the inner CompletableFuture.
   
   ### Modifications
   
   - Use `thenCompose` to instead of `thenAccept`
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   ### Documentation
   
   - [x] `doc-not-needed` 
   (Please explain why)


-- 
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] HQebupt commented on pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.

Posted by GitBox <gi...@apache.org>.
HQebupt commented on PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#issuecomment-1175086325

   /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] BewareMyPower commented on pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#issuecomment-1198428045

   /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] codelipenghui closed pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.

Posted by GitBox <gi...@apache.org>.
codelipenghui closed pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.
URL: https://github.com/apache/pulsar/pull/15811


-- 
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] mattisonchao closed pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.

Posted by GitBox <gi...@apache.org>.
mattisonchao closed pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.
URL: https://github.com/apache/pulsar/pull/15811


-- 
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 diff in pull request #15811: [fix][broker] Make `deleteTopicPolicies` serialized is executed when close topic.

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#discussion_r950995776


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2928,11 +2928,20 @@ public Optional<TopicPolicies> getTopicPolicies(TopicName topicName) {
     }
 
     public CompletableFuture<Void> deleteTopicPolicies(TopicName topicName) {
-        if (!pulsar().getConfig().isTopicLevelPoliciesEnabled()) {
+        final PulsarService pulsarService = pulsar();
+        if (!pulsarService.getConfig().isTopicLevelPoliciesEnabled()) {
             return CompletableFuture.completedFuture(null);
         }
-        TopicName cloneTopicName = TopicName.get(topicName.getPartitionedTopicName());
-        return pulsar.getTopicPoliciesService().deleteTopicPoliciesAsync(cloneTopicName);
+        return pulsarService.getPulsarResources().getNamespaceResources()
+                .getPoliciesAsync(topicName.getNamespaceObject())

Review Comment:
   Why do we need to get the namespace policy first?



-- 
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] mattisonchao closed pull request #15811: [fix][broker] Make `deleteTopicPolicies` serialized is executed when close topic.

Posted by GitBox <gi...@apache.org>.
mattisonchao closed pull request #15811: [fix][broker] Make `deleteTopicPolicies` serialized is executed when close topic.
URL: https://github.com/apache/pulsar/pull/15811


-- 
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] nodece commented on pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.

Posted by GitBox <gi...@apache.org>.
nodece commented on PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#issuecomment-1153207083

   Please rebase the master branch.


-- 
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 diff in pull request #15811: [fix][broker] Make `deleteTopicPolicies` serialized is executed when close topic.

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#discussion_r951132700


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthenticatedProducerConsumerTest.java:
##########
@@ -80,6 +80,7 @@ protected void setup() throws Exception {
         conf.setTlsCertificateFilePath(TLS_SERVER_CERT_FILE_PATH);
         conf.setTlsKeyFilePath(TLS_SERVER_KEY_FILE_PATH);
         conf.setTlsAllowInsecureConnection(true);
+        conf.setTopicLevelPoliciesEnabled(false);

Review Comment:
   @mattisonchao Please open a new issue for this one.



-- 
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] mattisonchao commented on a diff in pull request #15811: [fix][broker] Make `deleteTopicPolicies` serialized is executed when close topic.

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#discussion_r951066997


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2928,11 +2928,20 @@ public Optional<TopicPolicies> getTopicPolicies(TopicName topicName) {
     }
 
     public CompletableFuture<Void> deleteTopicPolicies(TopicName topicName) {
-        if (!pulsar().getConfig().isTopicLevelPoliciesEnabled()) {
+        final PulsarService pulsarService = pulsar();
+        if (!pulsarService.getConfig().isTopicLevelPoliciesEnabled()) {
             return CompletableFuture.completedFuture(null);
         }
-        TopicName cloneTopicName = TopicName.get(topicName.getPartitionedTopicName());
-        return pulsar.getTopicPoliciesService().deleteTopicPoliciesAsync(cloneTopicName);
+        return pulsarService.getPulsarResources().getNamespaceResources()
+                .getPoliciesAsync(topicName.getNamespaceObject())

Review Comment:
   We want to check if the namespace is marked as "deleted". This means the namespace is being deleted. We don't need to delete the topic policy anymore.
   If we connect to the `deleted` namespace. we will get an error.



-- 
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] mattisonchao commented on a diff in pull request #15811: [fix][broker] Make `deleteTopicPolicies` serialized is executed when close topic.

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#discussion_r951066997


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2928,11 +2928,20 @@ public Optional<TopicPolicies> getTopicPolicies(TopicName topicName) {
     }
 
     public CompletableFuture<Void> deleteTopicPolicies(TopicName topicName) {
-        if (!pulsar().getConfig().isTopicLevelPoliciesEnabled()) {
+        final PulsarService pulsarService = pulsar();
+        if (!pulsarService.getConfig().isTopicLevelPoliciesEnabled()) {
             return CompletableFuture.completedFuture(null);
         }
-        TopicName cloneTopicName = TopicName.get(topicName.getPartitionedTopicName());
-        return pulsar.getTopicPoliciesService().deleteTopicPoliciesAsync(cloneTopicName);
+        return pulsarService.getPulsarResources().getNamespaceResources()
+                .getPoliciesAsync(topicName.getNamespaceObject())

Review Comment:
   We want to check if the namespace is marked as "deleted". This means the namespace is being deleted. We don't need to delete the topic policy anymore.



-- 
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 #15811: [fix][broker] Make `deleteTopicPolicies` serialized is executed when close topic.

Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #15811:
URL: https://github.com/apache/pulsar/pull/15811


-- 
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] mattisonchao commented on a diff in pull request #15811: [fix][broker] Make `deleteTopicPolicies` serialized is executed when close topic.

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#discussion_r949732344


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthenticatedProducerConsumerTest.java:
##########
@@ -80,6 +80,7 @@ protected void setup() throws Exception {
         conf.setTlsCertificateFilePath(TLS_SERVER_CERT_FILE_PATH);
         conf.setTlsKeyFilePath(TLS_SERVER_KEY_FILE_PATH);
         conf.setTlsAllowInsecureConnection(true);
+        conf.setTopicLevelPoliciesEnabled(false);

Review Comment:
   Because TLS is not configured for the internal Pulsar client, we get an exception when we delete the topic policy.
   
   I'm wondering if we enable `topicPolicies` and TLS but don't provide any TLS configuration for the broker's internal clients.
   Do we have to check the broker`s internal client configuration when the broker starts?



-- 
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] mattisonchao closed pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.

Posted by GitBox <gi...@apache.org>.
mattisonchao closed pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.
URL: https://github.com/apache/pulsar/pull/15811


-- 
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 pull request #15811: [fix][broker] Avoid using `thenAccept` invoke async method.

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #15811:
URL: https://github.com/apache/pulsar/pull/15811#issuecomment-1153375016

   Looks like the change breaks lots of the tests, move to 2.10.2 first


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