Allow help flags to be specified for any Admin subcommand (apache#5220)

* Consolidates all subCommand Opts under Admin class

Moves the Opts class from ServiceStatusCmd and into the `Admin` class.

Removes the AdminOpts class

* Make call to stop explicit in admin command

Use `else if` instead of default `else` case as it will create a
client.shutdown rpc call to the manager.

* Update server/base/src/main/java/org/apache/accumulo/server/util/Admin.java

Automatically find all subcommand help options instead of formal IF statement.

Co-authored-by: Keith Turner <kturner@apache.org>

---------

Co-authored-by: Keith Turner <kturner@apache.org>

Author: ddanielr

core/src/test/java/org/apache/accumulo/core/cli/TestHelp.java

@@ -19,9 +19,12 @@
 package org.apache.accumulo.core.cli;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import org.junit.jupiter.api.Test;
 
+import com.beust.jcommander.Parameter;
+
 public class TestHelp {
   protected class HelpStub extends Help {
     @Override
@@ -46,4 +49,20 @@ public void testInvalidArgs() {
     }
   }
 
+  @Test
+  public void testHelpCommand() {
+    class TestHelpOpt extends HelpStub {
+      @Parameter(names = {"--test"})
+      boolean test = false;
+    }
+
+    String[] args = {"--help", "--test"};
+    TestHelpOpt opts = new TestHelpOpt();
+    try {
+      opts.parseArgs("program", args);
+      assertTrue(opts.test);
+    } catch (RuntimeException e) {
+      assertEquals("0", e.getMessage());
+    }
+  }
 }

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 {
+    @Parameter(names = {"-h", "-?", "--help", "-help"}, help = true)
+    public boolean help = false;
   }
 
   @Parameters(commandDescription = "stop the tablet server on the given hosts")
-  static class StopCommand {
+  static class StopCommand extends SubCommandOpts {
+    @Parameter(names = {"-f", "--force"},
+        description = "force the given server to stop by removing its lock")
+    boolean force = false;
     @Parameter(description = "<host> {<host> ... }")
     List<String> args = new ArrayList<>();
   }
 
   @Parameters(commandDescription = "Ping tablet servers.  If no arguments, pings all.")
-  static class PingCommand {
+  static class PingCommand extends SubCommandOpts {
     @Parameter(description = "{<host> ... }")
     List<String> args = new ArrayList<>();
   }
 
   @Parameters(commandDescription = "print tablets that are offline in online tables")
-  static class CheckTabletsCommand {
+  static class CheckTabletsCommand extends SubCommandOpts {
     @Parameter(names = "--fixFiles", description = "Remove dangling file pointers")
     boolean fixFiles = false;
 
@@ -125,17 +127,17 @@ static class CheckTabletsCommand {
   }
 
   @Parameters(commandDescription = "stop the manager")
-  static class StopManagerCommand {}
+  static class StopManagerCommand extends SubCommandOpts {}
 
   @Deprecated(since = "2.1.0")
   @Parameters(commandDescription = "stop the master (DEPRECATED -- use stopManager instead)")
   static class StopMasterCommand {}
 
   @Parameters(commandDescription = "stop all tablet servers and the manager")
-  static class StopAllCommand {}
+  static class StopAllCommand extends SubCommandOpts {}
 
   @Parameters(commandDescription = "list Accumulo instances in zookeeper")
-  static class ListInstancesCommand {
+  static class ListInstancesCommand extends SubCommandOpts {
     @Parameter(names = "--print-errors", description = "display errors while listing instances")
     boolean printErrors = false;
     @Parameter(names = "--print-all",
@@ -144,13 +146,13 @@ static class ListInstancesCommand {
   }
 
   @Parameters(commandDescription = "Accumulo volume utility")
-  static class VolumesCommand {
+  static class VolumesCommand extends SubCommandOpts {
     @Parameter(names = {"-l", "--list"}, description = "list volumes currently in use")
     boolean printErrors = false;
   }
 
   @Parameters(commandDescription = "print out non-default configuration settings")
-  static class DumpConfigCommand {
+  static class DumpConfigCommand extends SubCommandOpts {
     @Parameter(names = {"-a", "--all"},
         description = "print the system and all table configurations")
     boolean allConfiguration = false;
@@ -173,13 +175,13 @@ static class DumpConfigCommand {
           + "necessary.";
 
   @Parameters(commandDescription = RV_DEPRECATION_MSG)
-  static class RandomizeVolumesCommand {
+  static class RandomizeVolumesCommand extends SubCommandOpts {
     @Parameter(names = {"-t"}, description = "table to update", required = true)
     String tableName = null;
   }
 
   @Parameters(commandDescription = "Verify all Tablets are assigned to tablet servers")
-  static class VerifyTabletAssignmentsCommand {
+  static class VerifyTabletAssignmentsCommand extends SubCommandOpts {
     @Parameter(names = {"-v", "--verbose"},
         description = "verbose mode (prints locations of tablets)")
     boolean verbose = false;
@@ -194,14 +196,14 @@ static class ChangeSecretCommand {}
 
   @Parameters(
       commandDescription = "List or delete Tablet Server locks. Default with no arguments is to list the locks.")
-  static class TabletServerLocksCommand {
+  static class TabletServerLocksCommand extends SubCommandOpts {
     @Parameter(names = "-delete", description = "specify a tablet server lock to delete")
     String delete = null;
   }
 
   @Parameters(
       commandDescription = "Deletes specific instance name or id from zookeeper or cleans up all old instances.")
-  static class DeleteZooInstanceCommand {
+  static class DeleteZooInstanceCommand extends SubCommandOpts {
     @Parameter(names = {"-i", "--instance"}, description = "the instance name or id to delete")
     String instance;
     @Parameter(names = {"-c", "--clean"},
@@ -214,7 +216,7 @@ static class DeleteZooInstanceCommand {
   }
 
   @Parameters(commandDescription = "Restore Zookeeper data from a file.")
-  static class RestoreZooCommand {
+  static class RestoreZooCommand extends SubCommandOpts {
     @Parameter(names = "--overwrite")
     boolean overwrite = false;
 
@@ -224,7 +226,7 @@ static class RestoreZooCommand {
 
   @Parameters(commandNames = "fate",
       commandDescription = "Operations performed on the Manager FaTE system.")
-  static class FateOpsCommand {
+  static class FateOpsCommand extends SubCommandOpts {
     @Parameter(description = "[<txId>...]")
     List<String> txList = new ArrayList<>();
 
@@ -255,6 +257,15 @@ static class FateOpsCommand {
     List<String> states = new ArrayList<>();
   }
 
+  @Parameters(commandDescription = "show service status")
+  public static class ServiceStatusCmdOpts extends SubCommandOpts {
+    @Parameter(names = "--json", description = "provide output in json format (--noHosts ignored)")
+    boolean json = false;
+    @Parameter(names = "--noHosts",
+        description = "provide a summary of service counts without host details")
+    boolean noHosts = false;
+  }
+
   public static void main(String[] args) {
     new Admin().execute(args);
   }
@@ -277,13 +288,12 @@ public String description() {
   @SuppressFBWarnings(value = "DM_EXIT", justification = "System.exit okay for CLI tool")
   @Override
   public void execute(final String[] args) {
-    boolean everything;
 
-    AdminOpts opts = new AdminOpts();
+    ServerUtilOpts opts = new ServerUtilOpts();
     JCommander cl = new JCommander(opts);
     cl.setProgramName("accumulo admin");
 
-    ServiceStatusCmd.Opts serviceStatusCommandOpts = new ServiceStatusCmd.Opts();
+    ServiceStatusCmdOpts serviceStatusCommandOpts = new ServiceStatusCmdOpts();
     cl.addCommand("serviceStatus", serviceStatusCommandOpts);
 
     ChangeSecretCommand changeSecretCommand = new ChangeSecretCommand();
@@ -337,11 +347,21 @@ public void execute(final String[] args) {
 
     cl.parse(args);
 
-    if (opts.help || cl.getParsedCommand() == null) {
+    if (cl.getParsedCommand() == null) {
       cl.usage();
       return;
     }
 
+    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;
+        }
+      }
+    }
+
     ServerContext context = opts.getServerContext();
 
     AccumuloConfiguration conf = context.getConfiguration();
@@ -380,7 +400,7 @@ public void execute(final String[] args) {
         }
 
       } else if (cl.getParsedCommand().equals("stop")) {
-        stopTabletServer(context, stopOpts.args, opts.force);
+        stopTabletServer(context, stopOpts.args, stopOpts.force);
       } else if (cl.getParsedCommand().equals("dumpConfig")) {
         printConfig(context, dumpConfigCommand);
       } else if (cl.getParsedCommand().equals("volumes")) {
@@ -402,15 +422,19 @@ public void execute(final String[] args) {
       } else if (cl.getParsedCommand().equals("fate")) {
         executeFateOpsCommand(context, fateOpsCommand);
       } else if (cl.getParsedCommand().equals("serviceStatus")) {
-        printServiceStatus(context, serviceStatusCommandOpts);
-      } else {
-        everything = cl.getParsedCommand().equals("stopAll");
+        ServiceStatusCmd ssc = new ServiceStatusCmd();
+        ssc.execute(context, serviceStatusCommandOpts.json, serviceStatusCommandOpts.noHosts);
+      } else if (cl.getParsedCommand().equals("stopManager")
+          || cl.getParsedCommand().equals("stopAll")) {
+        boolean everything = cl.getParsedCommand().equals("stopAll");
 
         if (everything) {
           flushAll(context);
         }
 
         stopServer(context, everything);
+      } else {
+        cl.usage();
       }
 
       if (rc != 0) {
@@ -430,11 +454,6 @@ public void execute(final String[] args) {
     }
   }
 
-  private static void printServiceStatus(ServerContext context, ServiceStatusCmd.Opts opts) {
-    ServiceStatusCmd ssc = new ServiceStatusCmd();
-    ssc.execute(context, opts);
-  }
-
   private static int ping(ClientContext context, List<String> args) {
 
     InstanceOperations io = context.instanceOperations();

server/base/src/main/java/org/apache/accumulo/server/util/ServiceStatusCmd.java

@@ -39,8 +39,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.beust.jcommander.Parameter;
-import com.beust.jcommander.Parameters;
 import com.google.common.annotations.VisibleForTesting;
 
 public class ServiceStatusCmd {
@@ -56,7 +54,7 @@ public ServiceStatusCmd() {}
    * Read the service statuses from ZooKeeper, build the status report and then output the report to
    * stdout.
    */
-  public void execute(final ServerContext context, final Opts opts) {
+  public void execute(final ServerContext context, final boolean json, final boolean noHosts) {
 
     ZooReader zooReader = context.getZooReader();
 
@@ -74,9 +72,9 @@ public void execute(final ServerContext context, final Opts opts) {
     services.put(ServiceStatusReport.ReportKey.COMPACTOR, getCompactorStatus(zooReader, zooRoot));
     services.put(ServiceStatusReport.ReportKey.GC, getGcStatus(zooReader, zooRoot));
 
-    ServiceStatusReport report = new ServiceStatusReport(services, opts.noHosts);
+    ServiceStatusReport report = new ServiceStatusReport(services, noHosts);
 
-    if (opts.json) {
+    if (json) {
       System.out.println(report.toJson());
     } else {
       StringBuilder sb = new StringBuilder(8192);
@@ -336,15 +334,6 @@ Result<Set<String>> readAllNodesData(final ZooReader zooReader, final String pat
     return new Result<>(errorCount.get(), hosts);
   }
 
-  @Parameters(commandDescription = "show service status")
-  public static class Opts {
-    @Parameter(names = "--json", description = "provide output in json format (--noHosts ignored)")
-    boolean json = false;
-    @Parameter(names = "--noHosts",
-        description = "provide a summary of service counts without host details")
-    boolean noHosts = false;
-  }
-
   /**
    * Provides explicit method names instead of generic getFirst to get the error count and getSecond
    * hosts information

server/base/src/test/java/org/apache/accumulo/server/util/AdminCommandsTest.java

@@ -86,8 +86,8 @@ public void testDumpConfigCommand() {
 
   // not a command, but easy enough to include here
   @Test
-  public void testAdminOpts() {
-    Admin.AdminOpts opts = new Admin.AdminOpts();
+  public void testStopOpts() {
+    Admin.StopCommand opts = new Admin.StopCommand();
     assertFalse(opts.force);
   }
 }

server/base/src/test/java/org/apache/accumulo/server/util/ServiceStatusCmdTest.java

@@ -398,7 +398,7 @@ void zkNodeDeletedTest() throws Exception {
   @Test
   public void testServiceStatusCommandOpts() {
     replay(zooReader); // needed for @AfterAll verify
-    ServiceStatusCmd.Opts opts = new ServiceStatusCmd.Opts();
+    Admin.ServiceStatusCmdOpts opts = new Admin.ServiceStatusCmdOpts();
     assertFalse(opts.json);
     assertFalse(opts.noHosts);
   }