[GitHub] [hbase] wchevreuil commented on a change in pull request #2345: HBASE-24974: Provide a flexibility to print only row key and filter for multiple tables in the WALPrettyPrinter

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

wchevreuil commented on a change in pull request #2345:
URL: https://github.com/apache/hbase/pull/2345#discussion_r482819637



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALPrettyPrinter.java
##########
@@ -147,11 +153,13 @@ public void setSequenceFilter(long sequence) {
   }
 
   /**
-   * Sets the table filter. Only log entries for this table are printed.
-   * @param table table name to set.
+   * Sets the tables filter. Only log entries for these table are printed.

Review comment:
       nit: "Only lof entries for these tables..."

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALPrettyPrinter.java
##########
@@ -79,9 +81,12 @@
   private boolean outputJSON;
   // The following enable filtering by sequence, region, and row, respectively
   private long sequence;
-  private String table;
+
+  // List of tables for filter
+  private Set<String> tableList;

Review comment:
       nit: for clarity, shouldn't we name it as _tableSet_ instead of _tableList_? Or maybe better to use List/ArrayList.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALPrettyPrinter.java
##########
@@ -99,9 +104,10 @@ public WALPrettyPrinter() {
     outputValues = false;
     outputJSON = false;
     sequence = -1;
-    table = null;
+    tableList = new HashSet<>();

Review comment:
       Is HashSet really needed here? Can this be a simple ArrayList? 

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALPrettyPrinter.java
##########
@@ -391,6 +415,8 @@ public static void run(String[] args) throws IOException {
         "Region to filter by. Pass encoded region name; e.g. '9192caead6a5a20acb4454ffbc79fa14'");
     options.addOption("s", "sequence", true,
         "Sequence to filter by. Pass sequence number.");
+    options.addOption("k", "outputOnlyRowKey", false,
+      "If only row keys are required in output");

Review comment:
       Let's be more direct: "Prints only row keys".




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

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