Allow help flags to be specified for any Admin subcommand

[ddanielr]

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

[dlmarion]

Curious if you tried 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.

[keith-turner]

Tested locally and found one problem and made suggestion to fix it. Looks good otherwise.

[dlmarion]

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.

[dlmarion]

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.

[ddanielr]

Curious if you tried 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.

Specifically it attempts to parse the main param on line 819 before attempting the validators on line 822: https://github.com/cbeust/jcommander/blob/8ab73fd92c8ca25bd9358e8e6445ad9bff851747/src/main/java/com/beust/jcommander/JCommander.java#L819

[dlmarion]

Do we need this class? Could the other commands extend Help like ServerUtilOpts does?

[ddanielr]

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.

[dlmarion]

I wouldn't put any more work into it, just wondering if we could re-use the @Parameter declaration.

[dlmarion]

Looks good to me, just had a previous question about whether or not we could re-use the Help class.

[ddanielr]

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

[dlmarion]

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