[GitHub] [pulsar] codelipenghui commented on a change in pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription

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

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



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
##########
@@ -2731,7 +2731,7 @@ protected void internalSetMaxUnackedMessagesPerConsumer(int maxUnackedMessagesPe
         }
     }
 
-    protected int internalGetMaxUnackedMessagesPerSubscription() {
+    protected Integer internalGetMaxUnackedMessagesPerSubscription() {

Review comment:
       If we want to introduce some change that will break the namespace level policy behavior, is it need to start a PIP to discuss it and it should be a highlight in the release note. @sijie What do you think

##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/NamespacesImpl.java
##########
@@ -2452,6 +2452,30 @@ public void setMaxUnackedMessagesPerSubscription(String namespace, int maxUnacke
         return asyncPostRequest(path, Entity.entity(maxUnackedMessagesPerSubscription, MediaType.APPLICATION_JSON));
     }
 
+    @Override
+    public void removeMaxUnackedMessagesPerSubscription(String namespace)

Review comment:
       Do we need to also add a CLI command for this method?

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
##########
@@ -2751,19 +2751,19 @@ protected void internalSetMaxSubscriptionsPerTopic(Integer maxSubscriptionsPerTo
         internalSetPolicies("max_subscriptions_per_topic", maxSubscriptionsPerTopic);
     }
 
-    protected void internalSetMaxUnackedMessagesPerSubscription(int maxUnackedMessagesPerSubscription) {
+    protected void internalSetMaxUnackedMessagesPerSubscription(Integer maxUnackedMessagesPerSubscription) {
         validateNamespacePolicyOperation(namespaceName, PolicyName.MAX_UNACKED, PolicyOperation.WRITE);
         validatePoliciesReadOnlyAccess();
-
+        if (maxUnackedMessagesPerSubscription != null && maxUnackedMessagesPerSubscription < 0) {

Review comment:
       Currently, we use `null` to represent the absence of policy. How the users able to disable this policy for a specific namespace or topic?
   
   For example, if the broker enabled the `maxUnackedMessagesPerSubscription` but we want to disable it in the namespace level or topic level.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org