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 2021/01/23 14:24:41 UTC
[GitHub] [pulsar] 315157973 opened a new pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
315157973 opened a new pull request #9290:
URL: https://github.com/apache/pulsar/pull/9290
Master Issue: #<xyz>
### Modifications
1) Fix the unackedMessagesExceededOnSubscription at the namespace level cannot be disabled
2) Added applied interface
### Verifying this change
test applied api : testMaxUnackedMessagesOnSubApplied
test priority : testMaxUnackedMessagesOnSubscriptionPriority
----------------------------------------------------------------
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
[GitHub] [pulsar] codelipenghui merged pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #9290:
URL: https://github.com/apache/pulsar/pull/9290
----------------------------------------------------------------
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
[GitHub] [pulsar] codelipenghui commented on pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9290:
URL: https://github.com/apache/pulsar/pull/9290#issuecomment-790382708
Sorry for the late response @315157973 Could you please help resolve the conflicts? I think after this PR merged, all the applied policy is done right? Please help update the state https://github.com/apache/pulsar/issues/9216 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on a change in pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9290:
URL: https://github.com/apache/pulsar/pull/9290#discussion_r563226704
##########
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:
Just set it to 0, which is consistent with the description in broker.conf, but here it just cannot be set less than 0
----------------------------------------------------------------
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
[GitHub] [pulsar] 315157973 commented on pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9290:
URL: https://github.com/apache/pulsar/pull/9290#issuecomment-790383799
> Sorry for the late response @315157973 Could you please help resolve the conflicts? I think after this PR merged, all the applied policy is done right? Please help update the state #9216 thanks.
Sorry, there are 3 left, I will finish it as soon as possible this week
----------------------------------------------------------------
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
[GitHub] [pulsar] codelipenghui commented on a change in pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
Posted by 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
[GitHub] [pulsar] sijie commented on a change in pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9290:
URL: https://github.com/apache/pulsar/pull/9290#discussion_r563434214
##########
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:
It is better to have a PIP for this change. We should highlight it in the release notes.
----------------------------------------------------------------
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
[GitHub] [pulsar] sijie commented on a change in pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9290:
URL: https://github.com/apache/pulsar/pull/9290#discussion_r563434214
##########
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:
It is better to have a PIP for this change. We should highlight it in the release notes.
----------------------------------------------------------------
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
[GitHub] [pulsar] 315157973 commented on a change in pull request #9290: Support get topic applied policy for MaxUnackedMessagesPerSubscription
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9290:
URL: https://github.com/apache/pulsar/pull/9290#discussion_r563226712
##########
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:
Ok i will add
----------------------------------------------------------------
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