You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/06/15 06:20:21 UTC
[GitHub] [ozone] duongnguyen0 opened a new pull request, #3517: HDDS-6882: Correct exit code for invalid arguments passed to command-line tools.
duongnguyen0 opened a new pull request, #3517:
URL: https://github.com/apache/ozone/pull/3517
## What changes were proposed in this pull request?
Command-line tools don't take exit-code from `picocli` and set the exit-code accordingly. That makes exit-codes for cases like invalid argument be always 0.
This change fix that by replacing the use of the deprecated `parseWithHandler` method with the `execute` method that returns correct exit-code for parsing/invalid input cases.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6882
## How was this patch tested?
Regression is checked by unit test.
Manually tests to verify the expected status code for error-cases.
```
ozone sh bucket create /vol1/bucket1 -l 123
Invalid value for option '--layout': expected one of [FILE_SYSTEM_OPTIMIZED, OBJECT_STORE, LEGACY] (case-sensitive) but was '123'
Usage: ozone sh bucket create [-ghV] [-k=<bekName>] [-l=<allowedBucketLayout>]
[--namespace-quota=<quotaInNamespace>]
[--quota=<quotaInBytes>] [-r=<replication>]
[-t=<type>] [-u=<ownerName>] <value>
creates a bucket in a given volume
<value> URI of the volume/bucket.
Ozone URI could start with o3:// or without prefix.
URI may contain the host/serviceId and port of the
OM server. Both are optional. If they are not
specified it will be identified from the config
files.
-g, --enforcegdpr if true, indicates GDPR enforced bucket,
false/unspecified indicates otherwise
-h, --help Show this help message and exit.
-k, --bucketkey=<bekName>
bucket encryption key name
-l, --layout=<allowedBucketLayout>
Allowed Bucket Layouts: FILE_SYSTEM_OPTIMIZED,
OBJECT_STORE, LEGACY
--namespace-quota=<quotaInNamespace>
For volume this parameter represents the number of
buckets, and for buckets represents the number of
keys (eg. 5)
--quota, --space-quota=<quotaInBytes>
The maximum space quota can be used (eg. 1GB)
-r, --replication=<replication>
Replication definition. Valid values are replication
type-specific. For RATIS: ONE or THREE. In case
of EC, pass CODEC-DATA-PARITY-CHUNKSIZE, e.g.
rs-3-2-1024k, rs-6-3-1024k, rs-10-4-1024k
-t, --type, --replication-type=<type>
Replication type. Supported types are: RATIS, EC
-u, --user=<ownerName> Owner of the bucket. Defaults to current user if not
specified
-V, --version Print version information and exit.
sh-4.2$ echo $?
2
sh-4.2$ ozone sh bucket create /vol1/bucket_1
INVALID_BUCKET_NAME Bucket or Volume name has an unsupported character : _
sh-4.2$ echo $?
255
sh-4.2$ ozone sh bucket create /vol1/bucket1
2022-06-14 23:06:01,060 INFO rpc.RpcClient (RpcClient.java:createVolume(466)) - Creating Volume: vol1, with duong as owner and space quota set to -1 bytes, counts quota set to -1
sh-4.2$ echo $?
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3517:
URL: https://github.com/apache/ozone/pull/3517#discussion_r900313986
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java:
##########
@@ -52,6 +53,10 @@ public class GenericCli implements Callable<Void>, GenericParentCommand {
public GenericCli() {
cmd = new CommandLine(this);
+ cmd.setExecutionExceptionHandler((ex, commandLine, parseResult) -> {
+ printError(ex);
Review Comment:
Thanks @duongnguyen0 for the clear explanation.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swagle merged pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
swagle merged PR #3517:
URL: https://github.com/apache/ozone/pull/3517
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1156526498
Thanks @duongnguyen0 for working on this. Looks like there is an [unused import reported by checkstyle](https://github.com/apache/ozone/runs/6895145517#step:6:8) and [unit test failure in `TestStorageContainerManagerStarter`](https://github.com/apache/ozone/runs/6895145733#step:5:1226) seems related. Acceptance test failures are probably unrelated (I remember seeing them elsewhere, too).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1156792381
Change makes sense. Can you look into adding a test as this was not caught in our testing.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3517:
URL: https://github.com/apache/ozone/pull/3517#discussion_r899741257
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java:
##########
@@ -52,6 +53,10 @@ public class GenericCli implements Callable<Void>, GenericParentCommand {
public GenericCli() {
cmd = new CommandLine(this);
+ cmd.setExecutionExceptionHandler((ex, commandLine, parseResult) -> {
+ printError(ex);
Review Comment:
Thanks @duongnguyen0 for the contribution. Overall patch looks nice.
Existing [GenericCli.java#L88](https://github.com/apache/ozone/blob/master/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java#L88) `printError()` function is using exception cause, if its not null. Could you please check the print message in your patch and whether its giving proper err message. Probably you can compare it with the existing code especially on server exception cases.
`printError(ex.getCause() == null ? ex : ex.getCause());`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongnguyen0 commented on a diff in pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
duongnguyen0 commented on code in PR #3517:
URL: https://github.com/apache/ozone/pull/3517#discussion_r899809242
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java:
##########
@@ -52,6 +53,10 @@ public class GenericCli implements Callable<Void>, GenericParentCommand {
public GenericCli() {
cmd = new CommandLine(this);
+ cmd.setExecutionExceptionHandler((ex, commandLine, parseResult) -> {
+ printError(ex);
Review Comment:
Thanks for the thorough review @rakeshadr.
Indeed, the extraction of the ExecutionException's cause happens in the caller of ExecutionExceptionHandler, aka. `CommandLine.execute`.
```
public int execute(String... args) {
try {
parseResult[0] = parseArgs(args);
return enrichForBackwardsCompatibility(getExecutionStrategy()).execute(parseResult[0]);
} catch (ParameterException ex) {
...
} catch (ExecutionException ex) {
try {
Exception cause = ex.getCause() instanceof Exception ? (Exception) ex.getCause() : ex;
return getExecutionExceptionHandler().handleExecutionException(cause, ex.getCommandLine(), parseResult[0]);
} ....
}
```
Basically, the same printed error message is maintained. And I did a comparison to verify it and put a sample in the PR's description.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongnguyen0 commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
duongnguyen0 commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1157097409
> TestOzoneManagerStarter failure was masked by being skipped in previous run, sorry about that.
I updated the pull-requests with a new commit and that should trigger a rerun. Not sure if the `TestOzoneManagerStarter` would run properly this time. Please give me a hint if the test is still masked failure.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1157270202
> > `TestOzoneManagerStarter` failure was masked by being skipped in previous run, sorry about that.
>
> Not sure if the `TestOzoneManagerStarter` would run properly this time. Please give me a hint if the test is still masked failure.
Let me clarify my previous comment: [`TestOzoneManagerStarter` was skipped in previous run](https://github.com/apache/ozone/runs/6895145733#step:5:1588) because it is in a module that depends on the module where `TestStorageContainerManagerStarter` failed. Thus it was executed in the next run, triggered by 80c4421, which fixed the failing test. So we still need `TestOzoneManagerStarter` to be fixed.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3517:
URL: https://github.com/apache/ozone/pull/3517#discussion_r899741257
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java:
##########
@@ -52,6 +53,10 @@ public class GenericCli implements Callable<Void>, GenericParentCommand {
public GenericCli() {
cmd = new CommandLine(this);
+ cmd.setExecutionExceptionHandler((ex, commandLine, parseResult) -> {
+ printError(ex);
Review Comment:
Thanks @duongnguyen0 for the contribution. Overall patch looks nice.
Existing [GenericCli.java#L88](https://github.com/apache/ozone/blob/master/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java#L88) `printError()` function is using the exception cause, if its not null. Could you please check the print message in your patch and whether its printing all the necessary info.
`printError(ex.getCause() == null ? ex : ex.getCause());`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongnguyen0 commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
duongnguyen0 commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1157096080
> Can you look into adding a test as this was not caught in our testing.
Right, I added a simple robot test-suite to ensure correct CLI exit code.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongnguyen0 commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
duongnguyen0 commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1157297071
> Let me clarify my previous comment: [`TestOzoneManagerStarter` was skipped in previous run](https://github.com/apache/ozone/runs/6895145733#step:5:1588) because it is in a module that depends on the module where `TestStorageContainerManagerStarter` failed. Thus it was executed in the next run, triggered by [80c4421](https://github.com/apache/ozone/commit/80c442161c4dc8e1a968665cc092d7e310096cd6), which fixed the failing test. So we still need `TestOzoneManagerStarter` to be fixed.
My bad. I tried running the same unit-test script in local but somehow it didn't catch this. Updated `TestOzoneManagerStarter ` and manually checked all other places.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1156839443
Thanks @duongnguyen0 for updating the patch. `TestOzoneManagerStarter` failure was masked by being skipped in previous run, sorry about that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongnguyen0 commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
duongnguyen0 commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1156749642
> Thanks @duongnguyen0 for working on this. Looks like there is an [unused import reported by checkstyle](https://github.com/apache/ozone/runs/6895145517#step:6:8) and [unit test failure in `TestStorageContainerManagerStarter`](https://github.com/apache/ozone/runs/6895145733#step:5:1226) seems related. Acceptance test failures are probably unrelated (I remember seeing them elsewhere, too).
Thanks for having a look. I fixed the checkstyle problems and made necessary changes to the unit-tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swagle commented on pull request #3517: HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.
Posted by GitBox <gi...@apache.org>.
swagle commented on PR #3517:
URL: https://github.com/apache/ozone/pull/3517#issuecomment-1159119946
Thanks for thre review @kerneltime and @rakeshadr. Merging this
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org