[GitHub] [hbase-operator-tools] ndimiduk commented on a diff in pull request #105: HBASE-24587 hbck2 command should accept one or more files containing a list of region names/table names/namespaces

[GitBox <gi...@apache.org>]

ndimiduk commented on code in PR #105:
URL: https://github.com/apache/hbase-operator-tools/pull/105#discussion_r846418839


##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -196,6 +227,43 @@ int setRegionState(ClusterConnection connection, String region,
     return setRegionState(connection, region, 0, newState);
   }
 
+  int setRegionState(ClusterConnection connection, String[] args) throws IOException {
+    Options options = new Options();
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();
+    options.addOption(inputFile);
+    CommandLine commandLine = getCommandLine(args, options);
+    if (commandLine == null) {
+      return EXIT_FAILURE;
+    }
+    String[] argList = commandLine.getArgs();
+    if (argList == null) {
+      return EXIT_FAILURE;
+    }
+
+    if(!commandLine.hasOption(inputFile.getOpt())) {
+      String[] params = formatSetRegionStateCommand(argList);
+      return setRegionStateByArgs(connection, params);
+    } else {
+      List<String> inputList = getFromArgsOrFiles(stringArrayToList(argList), true);
+      for (String line : inputList) {
+        String[] params = formatSetRegionStateCommand(line.split("\\s+"));
+        if (setRegionStateByArgs(connection, params) == EXIT_FAILURE) {
+          showErrorMessage("setRegionState failed to set " + args);

Review Comment:
   `args` has `toString` called on it. `Array.toString()` is almost certainly not what you want. Wrap in `Arrays.toString()`



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -185,6 +185,37 @@ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOE
     }
   }
 
+  void setTableState(Hbck hbck, String[] args) throws IOException {
+    Options options = new Options();
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();
+    options.addOption(inputFile);
+    CommandLine commandLine = getCommandLine(args, options);
+    if (commandLine == null) {
+      return;
+    }
+    String[] argList = commandLine.getArgs();
+    if(!commandLine.hasOption(inputFile.getOpt())) {
+      System.out.println(setTableStateByArgs(hbck, argList));
+    } else {
+      List<String> inputList = getFromArgsOrFiles(stringArrayToList(argList), true);
+      for (String line : inputList) {
+        String[] params = line.split("\\s+");

Review Comment:
   Maybe you need a `trim` here as well.



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -1202,4 +1318,70 @@ public static void main(String [] args) throws Exception {
       System.exit(errCode);
     }
   }
+
+  private List<String> stringArrayToList(String... nameSpaceOrTable) {
+    return nameSpaceOrTable != null ? Arrays.asList(nameSpaceOrTable) : null;
+  }
+
+  /**
+   * Get list of input if no other options
+   * @param args Array of arguments
+   * @return the list of input from arguments or parsed from input files
+   */
+  private List<String> getInputList(String[] args) throws IOException {
+    if (args == null) {
+      return null;

Review Comment:
   You have a for-each loop over the value returned by this method, which means you will NPE. Probably you should return an empty list instead.



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -196,6 +227,43 @@ int setRegionState(ClusterConnection connection, String region,
     return setRegionState(connection, region, 0, newState);
   }
 
+  int setRegionState(ClusterConnection connection, String[] args) throws IOException {
+    Options options = new Options();
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();
+    options.addOption(inputFile);
+    CommandLine commandLine = getCommandLine(args, options);
+    if (commandLine == null) {
+      return EXIT_FAILURE;
+    }
+    String[] argList = commandLine.getArgs();
+    if (argList == null) {
+      return EXIT_FAILURE;
+    }
+
+    if(!commandLine.hasOption(inputFile.getOpt())) {
+      String[] params = formatSetRegionStateCommand(argList);
+      return setRegionStateByArgs(connection, params);
+    } else {
+      List<String> inputList = getFromArgsOrFiles(stringArrayToList(argList), true);
+      for (String line : inputList) {
+        String[] params = formatSetRegionStateCommand(line.split("\\s+"));

Review Comment:
   Should you have a `trim` in here as well?



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -384,27 +439,29 @@ int setRegionState(ClusterConnection connection, String region, int replicaId,
     options.addOption(recursive);
     Option wait = Option.builder("w").longOpt("lockWait").hasArg().type(Integer.class).build();
     options.addOption(wait);
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();
+    options.addOption(inputFile);
     // Parse command-line.
-    CommandLineParser parser = new DefaultParser();
-    CommandLine commandLine;
-    try {
-      commandLine = parser.parse(options, args, false);
-    } catch (ParseException e) {
-      showErrorMessage(e.getMessage());
+    CommandLine commandLine = getCommandLine(args, options);
+    if (commandLine == null) {
       return null;
     }
     long lockWait = DEFAULT_LOCK_WAIT;
     if (commandLine.hasOption(wait.getOpt())) {
       lockWait = Integer.parseInt(commandLine.getOptionValue(wait.getOpt()));
     }
-    String[] pidStrs = commandLine.getArgs();
+    boolean overrideFlag = commandLine.hasOption(override.getOpt());
+    boolean recursiveFlag = commandLine.hasOption(recursive.getOpt());
+    boolean inputFileFlag = commandLine.hasOption(inputFile.getOpt());
+
+    String[] pidStrs =getFromArgsOrFiles(commandLine.getArgList(), inputFileFlag)

Review Comment:
   Something is wrong here: IntelliJ tells me on line 459 that `pidStrs` is always null.



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -260,15 +328,22 @@ int setRegionState(ClusterConnection connection, String region, int replicaId,
     Options options = new Options();
     Option fixOption = Option.builder("f").longOpt("fix").build();
     options.addOption(fixOption);
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();

Review Comment:
   This is the 3rd repetition of this `Option` builder stuff. Should it be consolidated into a single place? Or maybe the `Option` api doesn't work that way and it's our program that is "strange" for having multiple option parsers.



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -260,15 +328,22 @@ int setRegionState(ClusterConnection connection, String region, int replicaId,
     Options options = new Options();
     Option fixOption = Option.builder("f").longOpt("fix").build();
     options.addOption(fixOption);
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();
+    options.addOption(inputFile);
+
     // Parse command-line.
-    CommandLineParser parser = new DefaultParser();
-    CommandLine commandLine;
-    commandLine = parser.parse(options, args, false);
+    CommandLine commandLine = getCommandLine(args, options);
+    if (commandLine == null) {
+      return null;

Review Comment:
   There's no `null` checking on the result returned by this method. I think you should return an empty map in this case, but that may have other unintended results. Please investigate this.



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -185,6 +185,37 @@ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOE
     }
   }
 
+  void setTableState(Hbck hbck, String[] args) throws IOException {
+    Options options = new Options();
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();
+    options.addOption(inputFile);
+    CommandLine commandLine = getCommandLine(args, options);
+    if (commandLine == null) {
+      return;
+    }
+    String[] argList = commandLine.getArgs();
+    if(!commandLine.hasOption(inputFile.getOpt())) {
+      System.out.println(setTableStateByArgs(hbck, argList));

Review Comment:
   Is this `System.out` stuff necessary for program operation, is it intended for the the user, or is it debugging for your development process? Normally we'd use a logger to print what looks to me like debugging information, but maybe there isn't one setup for this program, I haven't looked...



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -196,6 +227,43 @@ int setRegionState(ClusterConnection connection, String region,
     return setRegionState(connection, region, 0, newState);
   }
 
+  int setRegionState(ClusterConnection connection, String[] args) throws IOException {
+    Options options = new Options();
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();
+    options.addOption(inputFile);
+    CommandLine commandLine = getCommandLine(args, options);
+    if (commandLine == null) {
+      return EXIT_FAILURE;
+    }
+    String[] argList = commandLine.getArgs();
+    if (argList == null) {
+      return EXIT_FAILURE;
+    }
+
+    if(!commandLine.hasOption(inputFile.getOpt())) {
+      String[] params = formatSetRegionStateCommand(argList);
+      return setRegionStateByArgs(connection, params);
+    } else {
+      List<String> inputList = getFromArgsOrFiles(stringArrayToList(argList), true);
+      for (String line : inputList) {
+        String[] params = formatSetRegionStateCommand(line.split("\\s+"));
+        if (setRegionStateByArgs(connection, params) == EXIT_FAILURE) {
+          showErrorMessage("setRegionState failed to set " + args);
+        }
+      }
+      return EXIT_SUCCESS;
+    }
+  }
+
+  int setRegionStateByArgs(ClusterConnection connection, String[] args) throws IOException {
+    if (args == null || args.length < 3) {
+      return EXIT_FAILURE;
+    }
+    RegionState.State state = RegionState.State.valueOf(args[2]);
+    int replicaId = Integer.valueOf(args[1]);

Review Comment:
   I think that what you actually want here is `Integer.parseInt()`.



##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -260,15 +328,22 @@ int setRegionState(ClusterConnection connection, String region, int replicaId,
     Options options = new Options();
     Option fixOption = Option.builder("f").longOpt("fix").build();
     options.addOption(fixOption);
+    Option inputFile = Option.builder("i").longOpt("inputFiles").build();

Review Comment:
   Okay, I see: you're following the existing style. Disregard then.



-- 
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@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org