YARN-5697. Use CliParser to parse options in RMAdminCLI. Contributed by Tao jie

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/cli/RMAdminCLI.java

@@ -29,6 +29,12 @@
 import java.util.Map;
 import java.util.Set;
 
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.GnuParser;
+import org.apache.commons.cli.MissingArgumentException;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.Options;
+import org.apache.commons.cli.ParseException;
 import org.apache.hadoop.classification.InterfaceAudience.Private;
 import org.apache.hadoop.classification.InterfaceStability.Unstable;
 import org.apache.hadoop.conf.Configuration;
@@ -80,7 +86,6 @@ public class RMAdminCLI extends HAAdmin {
 
   private final RecordFactory recordFactory = 
     RecordFactoryProvider.getRecordFactory(null);
-  private boolean directlyAccessNodeLabelStore = false;
   static CommonNodeLabelsManager localNodeLabelsManager = null;
   private static final String NO_LABEL_ERR_MSG =
       "No cluster node-labels are specified";
@@ -130,8 +135,9 @@ public class RMAdminCLI extends HAAdmin {
               new UsageInfo("<label1,label2,label3> (label splitted by \",\")",
                   "remove from cluster node labels"))
           .put("-replaceLabelsOnNode",
-              new UsageInfo("[-failOnUnknownNodes] " +
+              new UsageInfo(
-                  "<\"node1[:port]=label1,label2 node2[:port]=label1,label2\">",
+                  "<\"node1[:port]=label1,label2 node2[:port]=label1,label2\"> "
+                  + "[-failOnUnknownNodes] ",
               "replace labels on nodes"
                   + " (please note that we do not support specifying multiple"
                   + " labels on a single host for now.)\n\t\t"
@@ -248,8 +254,9 @@ private static void printHelp(String cmd, boolean isHAEnabled) {
         " [-addToClusterNodeLabels <\"label1(exclusive=true),"
             + "label2(exclusive=false),label3\">]" +
         " [-removeFromClusterNodeLabels <label1,label2,label3>]" +
-        " [-replaceLabelsOnNode [-failOnUnknownNodes] "
+        " [-replaceLabelsOnNode " +
-            + "<\"node1[:port]=label1,label2 node2[:port]=label1\">]" +
+            "<\"node1[:port]=label1,label2 node2[:port]=label1\"> " +
+            "[-failOnUnknownNodes]]" +
         " [-directlyAccessNodeLabelStore]" +
         " [-refreshClusterMaxPriority]" +
         " [-updateNodeResource [NodeID] [MemSize] [vCores]" +
@@ -576,11 +583,26 @@ private Set<String> buildNodeLabelNamesFromStr(String args) {
     return labels;
   }
 
-  private int addToClusterNodeLabels(String args) throws IOException,
+  private int handleAddToClusterNodeLabels(String[] args, String cmd,
-      YarnException {
+      boolean isHAEnabled) throws IOException, YarnException, ParseException {
-    List<NodeLabel> labels = buildNodeLabelsFromStr(args);
+    Options opts = new Options();
+    opts.addOption("addToClusterNodeLabels", true,
+        "Add to cluster node labels.");
+    opts.addOption("directlyAccessNodeLabelStore", false,
+        "Directly access node label store.");
+    int exitCode = -1;
+    CommandLine cliParser = null;
+    try {
+      cliParser = new GnuParser().parse(opts, args);
+    } catch (MissingArgumentException ex) {
+      System.err.println(NO_LABEL_ERR_MSG);
+      printUsage(args[0], isHAEnabled);
+      return exitCode;
+    }
 
-    if (directlyAccessNodeLabelStore) {
+    List<NodeLabel> labels = buildNodeLabelsFromStr(
+        cliParser.getOptionValue("addToClusterNodeLabels"));
+    if (cliParser.hasOption("directlyAccessNodeLabelStore")) {
       getNodeLabelManagerInstance(getConf()).addToCluserNodeLabels(labels);
     } else {
       ResourceManagerAdministrationProtocol adminProtocol =
@@ -592,11 +614,26 @@ private int addToClusterNodeLabels(String args) throws IOException,
     return 0;
   }
 
-  private int removeFromClusterNodeLabels(String args) throws IOException,
+  private int handleRemoveFromClusterNodeLabels(String[] args, String cmd,
-      YarnException {
+      boolean isHAEnabled) throws IOException, YarnException, ParseException {
-    Set<String> labels = buildNodeLabelNamesFromStr(args);
+    Options opts = new Options();
+    opts.addOption("removeFromClusterNodeLabels", true,
+        "Remove From cluster node labels.");
+    opts.addOption("directlyAccessNodeLabelStore", false,
+        "Directly access node label store.");
+    int exitCode = -1;
+    CommandLine cliParser = null;
+    try {
+      cliParser = new GnuParser().parse(opts, args);
+    } catch (MissingArgumentException ex) {
+      System.err.println(NO_LABEL_ERR_MSG);
+      printUsage(args[0], isHAEnabled);
+      return exitCode;
+    }
 
-    if (directlyAccessNodeLabelStore) {
+    Set<String> labels = buildNodeLabelNamesFromStr(
+        cliParser.getOptionValue("removeFromClusterNodeLabels"));
+    if (cliParser.hasOption("directlyAccessNodeLabelStore")) {
       getNodeLabelManagerInstance(getConf()).removeFromClusterNodeLabels(
           labels);
     } else {
@@ -659,14 +696,35 @@ private Map<NodeId, Set<String>> buildNodeLabelsMapFromStr(String args) {
     return map;
   }
 
-  private int replaceLabelsOnNodes(String args, boolean failOnUnknownNodes)
+  private int handleReplaceLabelsOnNodes(String[] args, String cmd,
-      throws IOException, YarnException {
+      boolean isHAEnabled) throws IOException, YarnException, ParseException {
-    Map<NodeId, Set<String>> map = buildNodeLabelsMapFromStr(args);
+    Options opts = new Options();
-    return replaceLabelsOnNodes(map, failOnUnknownNodes);
+    opts.addOption("replaceLabelsOnNode", true,
+        "Replace label on node.");
+    opts.addOption("failOnUnknownNodes", false,
+        "Fail on unknown nodes.");
+    opts.addOption("directlyAccessNodeLabelStore", false,
+        "Directly access node label store.");
+    int exitCode = -1;
+    CommandLine cliParser = null;
+    try {
+      cliParser = new GnuParser().parse(opts, args);
+    } catch (MissingArgumentException ex) {
+      System.err.println(NO_MAPPING_ERR_MSG);
+      printUsage(args[0], isHAEnabled);
+      return exitCode;
+    }
+
+    Map<NodeId, Set<String>> map = buildNodeLabelsMapFromStr(
+        cliParser.getOptionValue("replaceLabelsOnNode"));
+    return replaceLabelsOnNodes(map,
+        cliParser.hasOption("failOnUnknownNodes"),
+        cliParser.hasOption("directlyAccessNodeLabelStore"));
   }
 
   private int replaceLabelsOnNodes(Map<NodeId, Set<String>> map,
-      boolean failOnUnknownNodes) throws IOException, YarnException {
+      boolean failOnUnknownNodes, boolean directlyAccessNodeLabelStore)
+      throws IOException, YarnException {
     if (directlyAccessNodeLabelStore) {
       getNodeLabelManagerInstance(getConf()).replaceLabelsOnNode(map);
     } else {
@@ -682,18 +740,6 @@ private int replaceLabelsOnNodes(Map<NodeId, Set<String>> map,
 
   @Override
   public int run(String[] args) throws Exception {
-    // -directlyAccessNodeLabelStore is a additional option for node label
-    // access, so just search if we have specified this option, and remove it
-    List<String> argsList = new ArrayList<String>();
-    for (int i = 0; i < args.length; i++) {
-      if (args[i].equals("-directlyAccessNodeLabelStore")) {
-        directlyAccessNodeLabelStore = true;
-      } else {
-        argsList.add(args[i]);
-      }
-    }
-    args = argsList.toArray(new String[0]);
-    
     YarnConfiguration yarnConf =
         getConf() == null ? new YarnConfiguration() : new YarnConfiguration(
             getConf());
@@ -766,37 +812,11 @@ public int run(String[] args) throws Exception {
       } else if ("-updateNodeResource".equals(cmd)) {
         exitCode = handleUpdateNodeResource(args, cmd, isHAEnabled);
       } else if ("-addToClusterNodeLabels".equals(cmd)) {
-        if (i >= args.length) {
+        exitCode = handleAddToClusterNodeLabels(args, cmd, isHAEnabled);
-          System.err.println(NO_LABEL_ERR_MSG);
-          printUsage("", isHAEnabled);
-          exitCode = -1;
-        } else {
-          exitCode = addToClusterNodeLabels(args[i]);
-        }
       } else if ("-removeFromClusterNodeLabels".equals(cmd)) {
-        if (i >= args.length) {
+        exitCode = handleRemoveFromClusterNodeLabels(args, cmd, isHAEnabled);
-          System.err.println(NO_LABEL_ERR_MSG);
-          printUsage("", isHAEnabled);
-          exitCode = -1;
-        } else {
-          exitCode = removeFromClusterNodeLabels(args[i]);
-        }
       } else if ("-replaceLabelsOnNode".equals(cmd)) {
-        if (i >= args.length) {
+        exitCode = handleReplaceLabelsOnNodes(args, cmd, isHAEnabled);
-          System.err.println(NO_MAPPING_ERR_MSG);
-          printUsage("", isHAEnabled);
-          exitCode = -1;
-        } else if ("-failOnUnknownNodes".equals(args[i])) {
-          if (i + 1 >= args.length) {
-            System.err.println(NO_MAPPING_ERR_MSG);
-            printUsage("", isHAEnabled);
-            exitCode = -1;
-          } else {
-            exitCode = replaceLabelsOnNodes(args[i + 1], true);
-          }
-        } else {
-          exitCode = replaceLabelsOnNodes(args[i], false);
-        }
       } else {
         exitCode = -1;
         System.err.println(cmd.substring(1) + ": Unknown command");
@@ -834,28 +854,47 @@ public int run(String[] args) throws Exception {
 
   // A helper method to reduce the number of lines of run()
   private int handleRefreshNodes(String[] args, String cmd, boolean isHAEnabled)
-      throws IOException, YarnException {
+      throws IOException, YarnException, ParseException {
-    if (args.length == 1) {
+    Options opts = new Options();
-      return refreshNodes();
+    opts.addOption("refreshNodes", false,
-    } else if (args.length == 3 || args.length == 4) {
+        "Refresh the hosts information at the ResourceManager.");
-      // if the graceful timeout specified
+    Option gracefulOpt = new Option("g", "graceful", true,
-      if ("-g".equals(args[1]) || "-graceful".equals(args[1])) {
+        "Wait for timeout before marking the NodeManager as decommissioned.");
+    gracefulOpt.setOptionalArg(true);
+    opts.addOption(gracefulOpt);
+    opts.addOption("client", false,
+        "Indicates the timeout tracking should be handled by the client.");
+    opts.addOption("server", false,
+        "Indicates the timeout tracking should be handled by the RM.");
+
+    int exitCode = -1;
+    CommandLine cliParser = null;
+    try {
+      cliParser = new GnuParser().parse(opts, args);
+    } catch (MissingArgumentException ex) {
+      System.out.println("Missing argument for options");
+      printUsage(args[0], isHAEnabled);
+      return exitCode;
+    }
+
     int timeout = -1;
-        String trackingMode;
+    if (cliParser.hasOption("g")) {
-        if (args.length == 4) {
+      String strTimeout = cliParser.getOptionValue("g");
-          timeout = validateTimeout(args[2]);
+      if (strTimeout != null) {
-          trackingMode = validateTrackingMode(args[3]);
+        timeout = validateTimeout(strTimeout);
+      }
+      String trackingMode = null;
+      if (cliParser.hasOption("client")) {
+        trackingMode = "client";
+      } else if (cliParser.hasOption("server")) {
+        trackingMode = "server";
       } else {
-          trackingMode = validateTrackingMode(args[2]);
+        printUsage(cmd, isHAEnabled);
+        return -1;
       }
       return refreshNodes(timeout, trackingMode);
     } else {
-        printUsage(cmd, isHAEnabled);
+      return refreshNodes();
-        return -1;
-      }
-    } else {
-      printUsage(cmd, isHAEnabled);
-      return -1;
     }
   }
 

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/cli/TestRMAdminCLI.java

@@ -469,8 +469,9 @@ public void testHelp() throws Exception {
               "[username]] [-addToClusterNodeLabels " +
               "<\"label1(exclusive=true),label2(exclusive=false),label3\">] " +
               "[-removeFromClusterNodeLabels <label1,label2,label3>] " +
-              "[-replaceLabelsOnNode [-failOnUnknownNodes] " +
+              "[-replaceLabelsOnNode " +
-              "<\"node1[:port]=label1,label2 node2[:port]=label1\">] " +
+              "<\"node1[:port]=label1,label2 node2[:port]=label1\"> " +
+              "[-failOnUnknownNodes]] " +
               "[-directlyAccessNodeLabelStore] [-refreshClusterMaxPriority] " +
               "[-updateNodeResource [NodeID] [MemSize] [vCores] " +
               "([OvercommitTimeout]) [-help [cmd]]"));
@@ -564,8 +565,8 @@ public void testHelp() throws Exception {
               + " [username]] [-addToClusterNodeLabels <\"label1(exclusive=true),"
                   + "label2(exclusive=false),label3\">]"
               + " [-removeFromClusterNodeLabels <label1,label2,label3>] [-replaceLabelsOnNode "
-              + "[-failOnUnknownNodes] "
+              + "<\"node1[:port]=label1,label2 node2[:port]=label1\"> "
-              + "<\"node1[:port]=label1,label2 node2[:port]=label1\">] [-directlyAccessNodeLabelStore] "
+              + "[-failOnUnknownNodes]] [-directlyAccessNodeLabelStore] "
               + "[-refreshClusterMaxPriority] "
               + "[-updateNodeResource [NodeID] [MemSize] [vCores] "
               + "([OvercommitTimeout]) "
@@ -614,13 +615,11 @@ public void testAccessLocalNodeLabelManager() throws Exception {
     dummyNodeLabelsManager.removeFromClusterNodeLabels(ImmutableSet.of("x", "y"));
     
     // change the sequence of "-directlyAccessNodeLabelStore" and labels,
-    // should not matter
+    // should fail
     args =
         new String[] { "-addToClusterNodeLabels",
             "-directlyAccessNodeLabelStore", "x,y" };
-    assertEquals(0, rmAdminCLI.run(args));
+    assertEquals(-1, rmAdminCLI.run(args));
-    assertTrue(dummyNodeLabelsManager.getClusterNodeLabelNames().containsAll(
-        ImmutableSet.of("x", "y")));
     
     // local node labels manager will be close after running
     assertTrue(dummyNodeLabelsManager.getServiceState() == STATE.STOPPED);
@@ -769,6 +768,10 @@ public void testReplaceLabelsOnNode() throws Exception {
     args = new String[] { "-replaceLabelsOnNode" };
     assertTrue(0 != rmAdminCLI.run(args));
 
+    // no labels, should fail
+    args = new String[] { "-replaceLabelsOnNode", "-failOnUnknownNodes" };
+    assertTrue(0 != rmAdminCLI.run(args));
+
     // no labels, should fail
     args =
         new String[] { "-replaceLabelsOnNode", "-directlyAccessNodeLabelStore" };