You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ddanielr (via GitHub)" <gi...@apache.org> on 2024/12/30 21:37:13 UTC
[PR] Allow help flags to be specified for any Admin subcommand [accumulo]
ddanielr opened a new pull request, #5220:
URL: https://github.com/apache/accumulo/pull/5220
Fixes #4755.
Modified the code to complete parsing first, then attempt to check for help flags with a second method call.
For most of the locations in the code, this check happens at the same point in time as it did previously.
Specifically for the admin code, the position of the `--help` flag does not matter for command options as it always returns the usage of the parsed command.
```
$ ./bin/accumulo admin serviceStatus --help
Usage: serviceStatus [options]
Options:
-h, -?, --help, -help
--json
provide output in json format (--noHosts ignored)
Default: false
--noHosts
provide a summary of service counts without host details
Default: false
$ ./bin/accumulo admin --help serviceStatus
Usage: serviceStatus [options]
Options:
-h, -?, --help, -help
--json
provide output in json format (--noHosts ignored)
Default: false
--noHosts
provide a summary of service counts without host details
Default: false
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#discussion_r1912236429
##########
server/base/src/main/java/org/apache/accumulo/server/util/Admin.java:
##########
@@ -337,11 +347,19 @@ public void execute(final String[] args) {
cl.parse(args);
- if (opts.help || cl.getParsedCommand() == null) {
+ if (cl.getParsedCommand() == null) {
cl.usage();
return;
}
+ if (opts.help || listInstancesOpts.help || pingCommand.help || checkTabletsCommand.help
+ || stopOpts.help || dumpConfigCommand.help || volumesCommand.help
+ || verifyTabletAssignmentsOpts.help || deleteZooInstOpts.help || restoreZooOpts.help
+ || fateOpsCommand.help || tServerLocksOpts.help || serviceStatusCommandOpts.help) {
+ cl.getCommands().get(cl.getParsedCommand()).usage();
+ return;
+ }
Review Comment:
I tried running this locally and it was really fast to only print help. I tried running `accumulo admin stopAll -h` and it did not print help and actually ran stopped stuff instead. I think that was because the command was not in the if stmt. Tried automatically finding stuff instead of the if stmt and that seemed to fix it.
```suggestion
for(var command : cl.getCommands().entrySet()){
var objects = command.getValue().getObjects();
for(var obj : objects) {
if(obj instanceof SubCommandOpts && ((SubCommandOpts)obj).help) {
command.getValue().usage();
return;
}
}
}
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#discussion_r1913410110
##########
server/base/src/main/java/org/apache/accumulo/server/util/Admin.java:
##########
@@ -96,26 +96,28 @@
public class Admin implements KeywordExecutable {
private static final Logger log = LoggerFactory.getLogger(Admin.class);
- static class AdminOpts extends ServerUtilOpts {
- @Parameter(names = {"-f", "--force"},
- description = "force the given server to stop by removing its lock")
- boolean force = false;
+ private static class SubCommandOpts {
Review Comment:
> Do we need this class? Could the other commands extend `Help` like `ServerUtilOpts` does?
They could, but the subcommands aren't using the jcommander object that is in the Help class so it seemed like a waste.
Alternatively, we could also remove this class and add the help params to each of the sub commands directly.
It's just a couple more lines of 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr merged PR #5220:
URL: https://github.com/apache/accumulo/pull/5220
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#discussion_r1913414758
##########
server/base/src/main/java/org/apache/accumulo/server/util/Admin.java:
##########
@@ -96,26 +96,28 @@
public class Admin implements KeywordExecutable {
private static final Logger log = LoggerFactory.getLogger(Admin.class);
- static class AdminOpts extends ServerUtilOpts {
- @Parameter(names = {"-f", "--force"},
- description = "force the given server to stop by removing its lock")
- boolean force = false;
+ private static class SubCommandOpts {
Review Comment:
I wouldn't put any more work into it, just wondering if we could re-use the `@Parameter` declaration.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#issuecomment-2587369193
> Curious if you tried [global parameter validation](https://jcommander.org/#_global_parameter_validation). It looks like you can create a validator class that is called when `parse` is called and JCommander provides you with all of the arguments. I'm thinking of the following case:
>
> 1. Create a class called `HelpRequestedParameterException` that extends `ParameterException`
> 2. Create a class that implements `IParametersValidator` that throws `HelpRequestedParameterException` if any of the keys in the parameter map are `-h`, `--help`, or `-?`.
> 3. Add the validator class to all of the `@Parameters` annotations.
> 4. Wrap the call to `JCommander.parse()` so that it traps `HelpRequestedParameterException` to display the help.
This seemed like a really neat way to handle the help param so I spent some time throwing it together.
However it doesn't work as Jcommander's parse first attempts to verify that each arg is a valid option and fails if it's not.
So if `help` isn't added to the sub command's opt string then it fails out.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#discussion_r1912236429
##########
server/base/src/main/java/org/apache/accumulo/server/util/Admin.java:
##########
@@ -337,11 +347,19 @@ public void execute(final String[] args) {
cl.parse(args);
- if (opts.help || cl.getParsedCommand() == null) {
+ if (cl.getParsedCommand() == null) {
cl.usage();
return;
}
+ if (opts.help || listInstancesOpts.help || pingCommand.help || checkTabletsCommand.help
+ || stopOpts.help || dumpConfigCommand.help || volumesCommand.help
+ || verifyTabletAssignmentsOpts.help || deleteZooInstOpts.help || restoreZooOpts.help
+ || fateOpsCommand.help || tServerLocksOpts.help || serviceStatusCommandOpts.help) {
+ cl.getCommands().get(cl.getParsedCommand()).usage();
+ return;
+ }
Review Comment:
I tried running this locally and it was really fast to only print help. I tried running `accumulo admin stopAll -h` and it did not print help and actually stopped stuff instead. I think that was because the command was not in the if stmt. Tried automatically finding stuff instead of the if stmt and that seemed to fix it.
```suggestion
for(var command : cl.getCommands().entrySet()){
var objects = command.getValue().getObjects();
for(var obj : objects) {
if(obj instanceof SubCommandOpts && ((SubCommandOpts)obj).help) {
command.getValue().usage();
return;
}
}
}
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#issuecomment-2587220493
just curious if my comment about global parameter validation was seen. I think that approach, if it works, would negate the need for every command to extend SubCommandOpts.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#issuecomment-2587500584
> > just curious if my comment about global parameter validation was seen. I think that approach, if it works, would negate the need for every command to extend SubCommandOpts.
>
> Re-reading the doc, maybe it won't work? It says, "After parsing your parameters with JCommander...", so if extra unknown args like `-h` or `-?` fail to parse, then it will never get to the parameter validation step.
If you are interested, I put up the changes for the validator exception: https://github.com/ddanielr/accumulo/pull/34/files#diff-4de8db0c0c757d544b8022eaeb6689358b6c94cda8dde6d57c2f366354fcb126
The `admin` subcommands work with the `--help` opts defined on each sub command.
I left them off of the `ec-admin` sub commands and they all fail out with the param exception
```
./accumulo ec-admin running --help
com.beust.jcommander.ParameterException: Was passed main parameter '--help' but no main parameter was defined in your arg class
at com.beust.jcommander.JCommander.initMainParameterValue(JCommander.java:1028)
...
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#discussion_r1913410110
##########
server/base/src/main/java/org/apache/accumulo/server/util/Admin.java:
##########
@@ -96,26 +96,28 @@
public class Admin implements KeywordExecutable {
private static final Logger log = LoggerFactory.getLogger(Admin.class);
- static class AdminOpts extends ServerUtilOpts {
- @Parameter(names = {"-f", "--force"},
- description = "force the given server to stop by removing its lock")
- boolean force = false;
+ private static class SubCommandOpts {
Review Comment:
> Do we need this class? Could the other commands extend `Help` like `ServerUtilOpts` does?
They could, but the subcommands aren't using the jcommander object that is in the Help command so it seemed like a waste.
Alternatively, we could also remove this class and add the help params to each of the sub commands directly.
It's just a couple more lines of 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#issuecomment-2587262716
> just curious if my comment about global parameter validation was seen. I think that approach, if it works, would negate the need for every command to extend SubCommandOpts.
Re-reading the doc, maybe it won't work? It says, "After parsing your parameters with JCommander...", so if extra unknown args like `-h` or `-?` fail to parse, then it will never get to the parameter validation step.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#issuecomment-2568316082
Curious if you tried [global parameter validation](https://jcommander.org/#_global_parameter_validation). It looks like you can create a validator class that is called when `parse` is called and JCommander provides you with all of the arguments. I'm thinking of the following case:
1. Create a class called `HelpRequestedParameterException` that extends `ParameterException`
2. Create a class that implements `IParametersValidator` that throws `HelpRequestedParameterException` if any of the keys in the parameter map are `-h`, `--help`, or `-?`.
3. Add the validator class to all of the `@Parameters` annotations.
4. Wrap the call to `JCommander.parse()` so that it traps `HelpRequestedParameterException` to display the help.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#issuecomment-2587536648
Oh, I see, you added the parameter **and** the validation. I think that looks a little cleaner, but I'll leave it up to you as whether or not to include 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#issuecomment-2587431549
> Re-reading the doc, maybe it won't work? It says, "After parsing your parameters with JCommander...", so if extra unknown args like `-h` or `-?` fail to parse, then it will never get to the parameter validation step.
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow help flags to be specified for any Admin subcommand [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5220:
URL: https://github.com/apache/accumulo/pull/5220#discussion_r1913399146
##########
server/base/src/main/java/org/apache/accumulo/server/util/Admin.java:
##########
@@ -96,26 +96,28 @@
public class Admin implements KeywordExecutable {
private static final Logger log = LoggerFactory.getLogger(Admin.class);
- static class AdminOpts extends ServerUtilOpts {
- @Parameter(names = {"-f", "--force"},
- description = "force the given server to stop by removing its lock")
- boolean force = false;
+ private static class SubCommandOpts {
Review Comment:
Do we need this class? Could the other commands extend `Help` like `ServerUtilOpts` does?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org