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

[pulsar-admin] Validate the size options in cmd for topic and namespace #13002

Merged
merged 2 commits into from
Nov 29, 2021
Merged

[pulsar-admin] Validate the size options in cmd for topic and namespace #13002

merged 2 commits into from
Nov 29, 2021

Conversation

yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Nov 27, 2021

Motivation

We should check the parameters releted to size and throw ParameterException when there is an exception, which can avoid displaying stack information on the client.
Currently, the lack of verification about size will directly display the exception, as below:

bin/pulsar-admin topics set-backlog-quota --limit 70z --policy producer_exception public/default/test
java.lang.NumberFormatException: For input string: "70z"
	at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
	at java.lang.Long.parseLong(Long.java:589)
	at java.lang.Long.parseLong(Long.java:631)
	at org.apache.pulsar.admin.cli.CliCommand.validateSizeString(CliCommand.java:102)
	at org.apache.pulsar.admin.cli.CmdTopics$SetBacklogQuota.run(CmdTopics.java:1148)
	at org.apache.pulsar.admin.cli.CmdBase.run(CmdBase.java:86)
	at org.apache.pulsar.admin.cli.PulsarAdminTool.run(PulsarAdminTool.java:282)
	at org.apache.pulsar.admin.cli.PulsarAdminTool.main(PulsarAdminTool.java:330)

Documentation

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 27, 2021
nicoloboschi
nicoloboschi requested changes Nov 27, 2021
View reviewed changes
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java Outdated
try {
sizeLimit = validateSizeString(limitStr);
} catch (IllegalArgumentException e) {
throw new ParameterException(String.format("Invalid retention policy type '%s'. Valid formats are: %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to specify the property with the invalid value, such as 'size limit'.

It's better to create a private method which build the error message, with property and valid values/examples as input

Jason918
Jason918 reviewed Nov 27, 2021
View reviewed changes
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java Outdated
long threshold;
try {
threshold = validateSizeString(thresholdStr);
} catch (IllegalArgumentException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to put the exception convert into validateSizeString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, PTAL

Jason918
Jason918 reviewed Nov 27, 2021
View reviewed changes
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
@@ -1808,12 +1801,13 @@ void run() throws PulsarAdminException {
description = "Maximum number of bytes in a topic backlog before compaction is triggered "
+ "(eg: 10M, 16G, 3T). 0 disables automatic compaction",
required = true)
private String threshold = "0";
private String thresholdStr = "0";
Copy link
Contributor

Choose a reason for hiding this comment

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

These "xxxxStr" modifications seem not necessary? IMHO, the previous way is cleaner.

Copy link
Contributor Author

@yuruguo yuruguo Nov 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the previous code is not easy to read, and it’s not a good way to pass the method in the method, I prefer to pass a variable. as below:

pulsar/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java

Lines 1517 to 1524 in cfda694

@Parameter(names = { "--size", "-s" }, description = "Retention size limit (eg: 10M, 16G, 3T). "
+ "0 or less than 1MB means no retention and -1 means infinite size retention", required = true)
private String limitStr;
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
long sizeLimit = validateSizeString(limitStr);

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, it is only a matter of taste.

@codelipenghui codelipenghui added this to the 2.10.0 milestone Nov 29, 2021
@merlimat merlimat merged commit 056ed2b into apache:master Nov 29, 2021
@merlimat merlimat added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/admin labels Nov 29, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@yuruguo
[pulsar-admin] Check the size options in cmd for topic and namespace (a…
5c7e68d 
…pache#13002)

* [pulsar-admin] Check the size options in cmd for topic and namespace

* Modify validateSizeString method
@codelipenghui codelipenghui changed the title [pulsar-admin] Check the size options in cmd for topic and namespace [pulsar-admin] Validate the size options in cmd for topic and namespace Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@Jason918 Jason918 Jason918 approved these changes

@merlimat merlimat merlimat approved these changes

Assignees

@yuruguo yuruguo

Labels
area/admin doc-not-needed Your PR changes do not impact docs type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@yuruguo @merlimat @Jason918 @codelipenghui @nicoloboschi