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/15 07:56:12 UTC
[GitHub] [pulsar] 315157973 opened a new pull request #9215: Keep topic-level policies commands consistent with that for namespace…
315157973 opened a new pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215
Fixes #9205
### Motivation
In #9108, we add some topic-level policies commands, and found some commands are not consistent with that for namespace-level.
For example,
On namespace-level, the policies commands are:
```
get-max-producers
set-max-producers
remove-max-producers
get-max-unacked-messages-per-subscription
set-max-unacked-messages-per-subscription
remove-max-unacked-messages-per-subscription
```
On topic-level, the polices commands are:
```
get-maxProducers
set-maxProducers
remove-maxProducers
get-max-unacked-messages-on-subscription
set-max-unacked-messages-on-subscription
remove-max-unacked-messages-on-subscription
```
### Modifications
Keep topic-level policies commands consistent with that for namespace
### Verifying this change
----------------------------------------------------------------
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 #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#discussion_r558052243
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -183,9 +182,9 @@ public CmdTopics(PulsarAdmin admin) {
jcommander.addCommand("set-inactive-topic-policies", new SetInactiveTopicPolicies());
jcommander.addCommand("remove-inactive-topic-policies", new RemoveInactiveTopicPolicies());
- jcommander.addCommand("get-max-consumers", new GetMaxConsumers());
- jcommander.addCommand("set-max-consumers", new SetMaxConsumers());
- jcommander.addCommand("remove-max-consumers", new RemoveMaxConsumers());
+ jcommander.addCommand("get-max-consumers-per-topic", new GetMaxConsumers());
+ jcommander.addCommand("set-max-consumers-per-topic", new SetMaxConsumers());
+ jcommander.addCommand("remove-max-consumers-per-topic", new RemoveMaxConsumers());
Review comment:
Ok i will restore it back
----------------------------------------------------------------
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 merged pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215
----------------------------------------------------------------
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 #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-765076072
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 removed a comment on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-761559229
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-767280844
@sijie It has been modified as required, please help CR again
----------------------------------------------------------------
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 #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#discussion_r558045590
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -183,9 +182,9 @@ public CmdTopics(PulsarAdmin admin) {
jcommander.addCommand("set-inactive-topic-policies", new SetInactiveTopicPolicies());
jcommander.addCommand("remove-inactive-topic-policies", new RemoveInactiveTopicPolicies());
- jcommander.addCommand("get-max-consumers", new GetMaxConsumers());
- jcommander.addCommand("set-max-consumers", new SetMaxConsumers());
- jcommander.addCommand("remove-max-consumers", new RemoveMaxConsumers());
+ jcommander.addCommand("get-max-consumers-per-topic", new GetMaxConsumers());
+ jcommander.addCommand("set-max-consumers-per-topic", new SetMaxConsumers());
+ jcommander.addCommand("remove-max-consumers-per-topic", new RemoveMaxConsumers());
Review comment:
Same as above comment
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -153,19 +152,19 @@ public CmdTopics(PulsarAdmin admin) {
jcommander.addCommand("get-compaction-threshold", new GetCompactionThreshold());
jcommander.addCommand("set-compaction-threshold", new SetCompactionThreshold());
jcommander.addCommand("remove-compaction-threshold", new RemoveCompactionThreshold());
- jcommander.addCommand("get-max-unacked-messages-on-consumer", new GetMaxUnackedMessagesOnConsumer());
- jcommander.addCommand("set-max-unacked-messages-on-consumer", new SetMaxUnackedMessagesOnConsumer());
- jcommander.addCommand("remove-max-unacked-messages-on-consumer", new RemoveMaxUnackedMessagesOnConsumer());
- jcommander.addCommand("get-max-unacked-messages-on-subscription", new GetMaxUnackedMessagesOnSubscription());
- jcommander.addCommand("set-max-unacked-messages-on-subscription", new SetMaxUnackedMessagesOnSubscription());
- jcommander.addCommand("remove-max-unacked-messages-on-subscription", new RemoveMaxUnackedMessagesOnSubscription());
+ jcommander.addCommand("get-max-unacked-messages-per-consumer", new GetMaxUnackedMessagesOnConsumer());
+ jcommander.addCommand("set-max-unacked-messages-per-consumer", new SetMaxUnackedMessagesOnConsumer());
+ jcommander.addCommand("remove-max-unacked-messages-per-consumer", new RemoveMaxUnackedMessagesOnConsumer());
+ jcommander.addCommand("get-max-unacked-messages-per-subscription", new GetMaxUnackedMessagesOnSubscription());
+ jcommander.addCommand("set-max-unacked-messages-per-subscription", new SetMaxUnackedMessagesOnSubscription());
+ jcommander.addCommand("remove-max-unacked-messages-per-subscription", new RemoveMaxUnackedMessagesOnSubscription());
jcommander.addCommand("get-publish-rate", new GetPublishRate());
jcommander.addCommand("set-publish-rate", new SetPublishRate());
jcommander.addCommand("remove-publish-rate", new RemovePublishRate());
- jcommander.addCommand("get-maxProducers", new GetMaxProducers());
- jcommander.addCommand("set-maxProducers", new SetMaxProducers());
- jcommander.addCommand("remove-maxProducers", new RemoveMaxProducers());
+ jcommander.addCommand("get-max-producers-per-topic", new GetMaxProducers());
+ jcommander.addCommand("set-max-producers-per-topic", new SetMaxProducers());
+ jcommander.addCommand("remove-max-producers-per-topic", new RemoveMaxProducers());
Review comment:
For the topic level policy, we don't need `-per-topic`
----------------------------------------------------------------
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 #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#discussion_r558102540
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -120,8 +120,7 @@ public CmdTopics(PulsarAdmin admin) {
jcommander.addCommand("set-retention", new SetRetention());
jcommander.addCommand("remove-retention", new RemoveRetention());
- jcommander.addCommand("enable-deduplication", new EnableDeduplication());
- jcommander.addCommand("disable-deduplication", new DisableDeduplication());
+ jcommander.addCommand("set-deduplication", new SetDeduplication());
jcommander.addCommand("get-deduplication-enabled", new GetDeduplicationEnabled());
Review comment:
Well, it sounds good
----------------------------------------------------------------
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 removed a comment on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-765076072
----------------------------------------------------------------
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 #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-761559229
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-765347103
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-760786590
> @315157973 Thank you for your contribution. Have we taken compatibilities into consideration in this PR?
> I mean, for example, both `get-maxProducer` and `get-max-producer` commands are available for existing users (`get-maxProducer`) and new users (`get-max-producer`). When users check the topics subcommands in terminal, only `get-max-producer` will be listed.
>
> @sijia-w could you also help review this PR? Thank you.
Regarding compatibility, I have discussed with @codelipenghui, considering that CLI is always input by humans (either by looking at the document to copy and paste, or through the help command), direct modification does not have a big impact.
We have not changed the API interface.
----------------------------------------------------------------
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 #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-761581957
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-765324613
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jennifer88huang commented on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-760774403
@315157973 Thank you for your contribution. Have we taken compatibilities into consideration in this PR?
I mean, for example, both `get-maxProducer` and `get-max-producer` commands are available for existing users (`get-maxProducer`) and new users (`get-max-producer`). When users check the topics subcommands in terminal, only `get-max-producer` will be listed.
@sijia-w could you also help review this PR? Thank you.
----------------------------------------------------------------
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] Jennifer88huang commented on a change in pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on a change in pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#discussion_r558087341
##########
File path: pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java
##########
@@ -764,10 +764,7 @@ public void topics() throws Exception {
cmdTopics.run(split("peek-messages persistent://myprop/clust/ns1/ds1 -s sub1 -n 3"));
verify(mockTopics).peekMessages("persistent://myprop/clust/ns1/ds1", "sub1", 3);
- cmdTopics.run(split("enable-deduplication persistent://myprop/clust/ns1/ds1"));
- verify(mockTopics).enableDeduplication("persistent://myprop/clust/ns1/ds1", true);
-
- cmdTopics.run(split("disable-deduplication persistent://myprop/clust/ns1/ds1"));
+ cmdTopics.run(split("set-deduplication persistent://myprop/clust/ns1/ds1 --disable"));
verify(mockTopics).enableDeduplication("persistent://myprop/clust/ns1/ds1", false);
Review comment:
since we refine "enable-deduplication" as "set-deduplication", we still have "get-deduplication-enabled" below. What's the differences?
----------------------------------------------------------------
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] Jennifer88huang commented on a change in pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on a change in pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#discussion_r558093676
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -120,8 +120,7 @@ public CmdTopics(PulsarAdmin admin) {
jcommander.addCommand("set-retention", new SetRetention());
jcommander.addCommand("remove-retention", new RemoveRetention());
- jcommander.addCommand("enable-deduplication", new EnableDeduplication());
- jcommander.addCommand("disable-deduplication", new DisableDeduplication());
+ jcommander.addCommand("set-deduplication", new SetDeduplication());
jcommander.addCommand("get-deduplication-enabled", new GetDeduplicationEnabled());
Review comment:
shall we use "get-deduplication-enabled" or "get-deduplication"?
----------------------------------------------------------------
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 removed a comment on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-761539911
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on a change in pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#discussion_r558090550
##########
File path: pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java
##########
@@ -764,10 +764,7 @@ public void topics() throws Exception {
cmdTopics.run(split("peek-messages persistent://myprop/clust/ns1/ds1 -s sub1 -n 3"));
verify(mockTopics).peekMessages("persistent://myprop/clust/ns1/ds1", "sub1", 3);
- cmdTopics.run(split("enable-deduplication persistent://myprop/clust/ns1/ds1"));
- verify(mockTopics).enableDeduplication("persistent://myprop/clust/ns1/ds1", true);
-
- cmdTopics.run(split("disable-deduplication persistent://myprop/clust/ns1/ds1"));
+ cmdTopics.run(split("set-deduplication persistent://myprop/clust/ns1/ds1 --disable"));
verify(mockTopics).enableDeduplication("persistent://myprop/clust/ns1/ds1", false);
Review comment:
There is no corresponding query interface at the namespace-level, which needs to be added later
----------------------------------------------------------------
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 #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-765076072
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jennifer88huang edited a comment on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
Jennifer88huang edited a comment on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-760897036
@315157973 Thank you for your clarification. Do not merge it before any eng approves it.
----------------------------------------------------------------
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] Jennifer88huang commented on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-760897036
@315157973 Thank you for your clarification.
----------------------------------------------------------------
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 #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-761539911
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9215: Keep topic-level policies commands consistent with that for namespace…
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9215:
URL: https://github.com/apache/pulsar/pull/9215#issuecomment-765225674
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org