[GitHub] [hbase] saintstack commented on a change in pull request #2282: HBASE-24901 Create versatile hbase-shell table formatter

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

saintstack commented on a change in pull request #2282:
URL: https://github.com/apache/hbase/pull/2282#discussion_r473307133



##########
File path: hbase-shell/src/main/ruby/hbase/taskmonitor.rb
##########
@@ -107,88 +107,5 @@ def tasksOnHost(filter, host)
 
       tasks
     end
-
-    #---------------------------------------------------------------------------------------------------
-    # Prints a table of filtered tasks on requested hosts
-    def tasks(filter, hosts)

Review comment:
       unused? Or you use your new formatter now?

##########
File path: hbase-shell/src/main/ruby/hbase/table.rb
##########
@@ -859,7 +892,9 @@ def convert_bytes_with_position(bytes, offset, len, converter_class, converter_m
     # 1. return back normal column pair as usual, i.e., "cf:qualifier[:CONVERTER]" to "cf" and "qualifier" only
     # 2. register the CONVERTER information based on column spec - "cf:qualifier"
     #
-    # Deprecated for removal in 4.0.0
+    # Deprecated for removal in 4.0.0. This method used to be used for both parsing and updating
+    # the @converters. This has been separated more cleanly into two new methods,
+    # parse_column_format_spec and set_column_converter. Please use them instead.

Review comment:
       Nice comment

##########
File path: hbase-shell/src/main/ruby/shell/formatter.rb
##########
@@ -192,6 +199,56 @@ def footer(row_count = nil, is_stale = false)
       end
     end
 
+    # Base abstract class for results formatting.

Review comment:
       Doc on how to do an impl?

##########
File path: hbase-shell/src/main/ruby/shell/commands/list_peers.rb
##########
@@ -38,10 +38,10 @@ def help
       def command
         peers = replication_admin.list_peers
 
-        formatter.header(%w[PEER_ID CLUSTER_KEY ENDPOINT_CLASSNAME
+        table_formatter.start_table({ num_cols: 11, headers: %w[PEER_ID CLUSTER_KEY ENDPOINT_CLASSNAME

Review comment:
       The old didn't bother passing  count of cols.... Would the passed header count ever differ from col count?




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