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.

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

[improve][cli] improve admin set-backlog-quota more clear #19300

Merged
merged 3 commits into from
Feb 2, 2023

Conversation

labuladong
Copy link
Contributor

@labuladong labuladong commented Jan 20, 2023

Fixes #19196

Motivation

The parameters of backlog retention policies need to be clarified. Size limit is only useful in destination_storage and time limit is only useful in message_age.

Modifications

  1. Make --limit optional, because message_age doesn't need this parameter but this parameter is required now.
  2. When the quota type is message_age, use the time limit and ignore the size limit. When the quota type is destination_storage, use the size limit and ignore the time limit. Don't set them both, which can be confusing for users.

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

doc update: apache/pulsar-site#394

Matching PR in forked repository

PR in forked repository: labuladong#19

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jan 20, 2023
@github-actions github-actions bot added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Jan 31, 2023
@Technoboy- Technoboy- closed this Feb 2, 2023
@Technoboy- Technoboy- reopened this Feb 2, 2023
@Technoboy- Technoboy- added this to the 3.0.0 milestone Feb 2, 2023
Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -977,8 +977,8 @@ private class SetBacklogQuota extends CliCommand {
private String policyStr;

@Parameter(names = {"-t", "--type"}, description = "Backlog quota type to set. Valid options are: "
+ "destination_storage and message_age. "
+ "destination_storage limits backlog by size (in bytes). "
+ "destination_storage (default) and message_age. "
Copy link
Contributor

Choose a reason for hiding this comment

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

The default is backlog size, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@codelipenghui codelipenghui merged commit 31fe347 into apache:master Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-complete Your PR changes impact docs and the related docs have been already added. ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Doc] 2.10* docs provide incorrect command for namespace set-backlog-quota
3 participants