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/06/16 14:15:47 UTC

[GitHub] [pulsar] eolivelli opened a new pull request, #16095: PIP-105: new API to get subscription properties

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

   ### Motivation
   
   In PIP-105 we added support for Subscription Properties. You can read them using the "stats" command, but it is not very user friendly.
   
   ### Modifications
   
   This patch adds a specific REST API to get the properties and a pulsar-admin command
   
   ### Verifying this change
   
   This change added tests 
   
   ### Documentation
   
   Check the box below or label this PR directly.
   
   Need to update docs? 
   
   - [x] `doc-not-needed` 
   (Please explain why)
     
   REST API and pulsar-admin docs are generated automatically


-- 
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] Jason918 commented on a diff in pull request #16095: PIP-105: new API to get subscription properties

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -1614,6 +1614,35 @@ private void internalUpdateSubscriptionPropertiesForNonPartitionedTopic(AsyncRes
                 });
     }
 
+    private void internalGetSubscriptionPropertiesForNonPartitionedTopic(AsyncResponse asyncResponse,
+                                                                            String subName,
+                                                                            boolean authoritative) {
+        validateTopicOwnershipAsync(topicName, authoritative)
+                .thenRun(() -> validateTopicOperation(topicName, TopicOperation.CONSUME))
+                .thenApply(__ -> {
+                    Topic topic = getTopicReference(topicName);

Review Comment:
   better to use `getTopicReferenceAsync()`?



##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java:
##########
@@ -98,6 +99,7 @@ public CmdTopics(Supplier<PulsarAdmin> admin) {
         jcommander.addCommand("unsubscribe", new DeleteSubscription());
         jcommander.addCommand("create-subscription", new CreateSubscription());
         jcommander.addCommand("update-subscription-properties", new UpdateSubscriptionProperties());
+        jcommander.addCommand("update-subscription-properties", new GetSubscriptionProperties());

Review Comment:
   ```suggestion
           jcommander.addCommand("get-subscription-properties", new GetSubscriptionProperties());
   ```



-- 
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 diff in pull request #16095: PIP-105: new API to get subscription properties

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -1614,6 +1614,35 @@ private void internalUpdateSubscriptionPropertiesForNonPartitionedTopic(AsyncRes
                 });
     }
 
+    private void internalGetSubscriptionPropertiesForNonPartitionedTopic(AsyncResponse asyncResponse,
+                                                                            String subName,
+                                                                            boolean authoritative) {
+        validateTopicOwnershipAsync(topicName, authoritative)
+                .thenRun(() -> validateTopicOperation(topicName, TopicOperation.CONSUME))
+                .thenApply(__ -> {
+                    Topic topic = getTopicReference(topicName);

Review Comment:
   good catch !



##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java:
##########
@@ -98,6 +99,7 @@ public CmdTopics(Supplier<PulsarAdmin> admin) {
         jcommander.addCommand("unsubscribe", new DeleteSubscription());
         jcommander.addCommand("create-subscription", new CreateSubscription());
         jcommander.addCommand("update-subscription-properties", new UpdateSubscriptionProperties());
+        jcommander.addCommand("update-subscription-properties", new GetSubscriptionProperties());

Review Comment:
   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] eolivelli merged pull request #16095: PIP-105: new API to get subscription properties

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


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