Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Keep topic-level policies commands consistent with that for namespace… #9215

Merged
merged 7 commits into from
Jan 26, 2021
Merged

Keep topic-level policies commands consistent with that for namespace… #9215

merged 7 commits into from
Jan 26, 2021

Conversation

315157973
Copy link
Contributor

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

codelipenghui
codelipenghui reviewed Jan 15, 2021
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good me, just left some minor comments. Since this is only breaking the CLI, I think it's not a serious problem.

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java Outdated Show resolved Hide resolved
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java Outdated Show resolved Hide resolved
@codelipenghui codelipenghui added this to the 2.8.0 milestone Jan 15, 2021
@codelipenghui codelipenghui added the doc-required Your PR changes impact docs and you will update later. label Jan 15, 2021
@Jennifer88huang-zz
Copy link
Contributor

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

@315157973
Copy link
Contributor Author

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

@Jennifer88huang-zz
Copy link
Contributor

Jennifer88huang-zz commented Jan 15, 2021 •
edited
Loading

@315157973 Thank you for your clarification. Do not merge it before any eng approves it.

@315157973
Copy link
Contributor Author

/pulsarbot run-failure-checks

sijie
sijie reviewed Jan 18, 2021
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@315157973 @codelipenghui If the commands of topic level polices were introduced in any major releases, we should keep the backward compatibility. You can add the new commands and mark the old commands as deprecated and hidden.

codelipenghui
codelipenghui approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@315157973
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@315157973
Copy link
Contributor Author

/pulsarbot run-failure-checks

@315157973
Copy link
Contributor Author

@sijie It has been modified as required, please help CR again

@sijie sijie merged commit d557e0a into apache:master Jan 26, 2021
codelipenghui pushed a commit that referenced this pull request Jan 26, 2021
@315157973 @codelipenghui
Keep topic-level policies commands consistent with that for namespace… (
4fc2e07 
#9215)

Fixes #9205
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
```

Keep topic-level policies commands consistent with that for namespace

(cherry picked from commit d557e0a)
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jan 26, 2021
This was referenced Jan 29, 2021
Merged
Closed
@Anonymitaet Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Feb 18, 2021
@315157973 315157973 deleted the consistent branch March 12, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sijie sijie sijie approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@Jennifer88huang-zz Jennifer88huang-zz Jennifer88huang-zz approved these changes

@jiazhai jiazhai Awaiting requested review from jiazhai

Assignees

@315157973 315157973

Labels
cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.1
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

Keep topic-level policies commands consistent with that for namespace level
5 participants
@315157973 @Jennifer88huang-zz @sijie @codelipenghui @Anonymitaet