HBASE-24722 Update commands with unintentional return values (#2058)

- Prior to this commit, there were 13 commands that unintentionally return the
  number of lines they print (usually one). This commit ensures that they
  return the value documented by the help text, or nil if there is not a simple
  logical value to return.
- Fixes 6 hbase-shell commands that return String rather than TrueClass or
  FalseClass
- Use double-bang to cast truthy values to TrueClass and FalseClass so that
  ruby's to_s can reliably print true or false without using ternary operators
- Updates tests for is_disabled, is_enabled, disable_rpc_throttle,
  enable_rpc_throttle, disable_exceed_throttle_quota,
  enable_exceed_throttle_quota, clear_deadservers, snapshot_cleanup_switch,
  snapshot_cleanup_enabled, and balancer to check return values
- Adds new tests for balance_switch, balancer_enabled, normalizer_switch,
  normalizer_enabled, catalog_janitor_switch, catalogjanitor_enabled,
  cleaner_chore_switch, cleaner_chore_enabled, splitormerge_switch, and
  splitormerge_enabled

signed-off-by: stack <stack@apache.org>

hbase-shell/src/main/ruby/shell/commands/balance_switch.rb

@@ -31,7 +31,7 @@ EOF
       end
 
       def command(enableDisable)
-        prev_state = admin.balance_switch(enableDisable) ? 'true' : 'false'
+        prev_state = !!admin.balance_switch(enableDisable)
         formatter.row(["Previous balancer state : #{prev_state}"])
         prev_state
       end

hbase-shell/src/main/ruby/shell/commands/balancer.rb

@@ -44,7 +44,9 @@ EOF
         elsif !force.nil?
           raise ArgumentError, "Invalid argument #{force}."
         end
-        formatter.row([admin.balancer(force_balancer) ? 'true' : 'false'])
+        did_balancer_run = !!admin.balancer(force_balancer)
+        formatter.row([did_balancer_run.to_s])
+        did_balancer_run
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/catalogjanitor_enabled.rb

@@ -29,7 +29,9 @@ EOF
       end
 
       def command
-        formatter.row([admin.catalogjanitor_enabled ? 'true' : 'false'])
+        current_state = !!admin.catalogjanitor_enabled
+        formatter.row([current_state.to_s])
+        current_state
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/catalogjanitor_switch.rb

@@ -30,7 +30,9 @@ EOF
       end
 
       def command(enableDisable)
-        formatter.row([admin.catalogjanitor_switch(enableDisable) ? 'true' : 'false'])
+        previous_state = !!admin.catalogjanitor_switch(enableDisable)
+        formatter.row([previous_state.to_s])
+        previous_state
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/cleaner_chore_enabled.rb

@@ -29,7 +29,9 @@ EOF
       end
 
       def command
-        formatter.row([admin.cleaner_chore_enabled ? 'true' : 'false'])
+        current_state = !!admin.cleaner_chore_enabled
+        formatter.row([current_state.to_s])
+        current_state
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/cleaner_chore_switch.rb

@@ -30,7 +30,9 @@ EOF
       end
 
       def command(enableDisable)
-        formatter.row([admin.cleaner_chore_switch(enableDisable) ? 'true' : 'false'])
+        previous_state = !!admin.cleaner_chore_switch(enableDisable)
+        formatter.row([previous_state.to_s])
+        previous_state
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/clear_block_cache.rb

@@ -33,6 +33,7 @@ EOF
 
       def command(table_name)
         formatter.row([admin.clear_block_cache(table_name)])
+        nil
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/clear_deadservers.rb

@@ -22,7 +22,9 @@ module Shell
     class ClearDeadservers < Command
       def help
         <<-EOF
-          Clear the dead region servers that are never used.
+          Clear the dead region servers that are never used. Returns an array containing any
+          deadservers that could not be cleared.
+
           Examples:
           Clear all dead region servers:
           hbase> clear_deadservers
@@ -35,18 +37,20 @@ module Shell
       end
 
       # rubocop:disable Metrics/AbcSize
-      # rubocop:disable Metrics/MethodLength
       def command(*dead_servers)
         servers = admin.clear_deadservers(dead_servers)
         if servers.size <= 0
           formatter.row(['true'])
+          []
         else
           formatter.row(['Some dead server clear failed'])
           formatter.row(['SERVERNAME'])
-          servers.each do |server|
-            formatter.row([server.toString])
+          server_names = servers.map { |server| server.toString }
+          server_names.each do |server|
+            formatter.row([server])
           end
           formatter.footer(servers.size)
+          server_names
         end
       end
       # rubocop:enable Metrics/AbcSize

hbase-shell/src/main/ruby/shell/commands/disable_exceed_throttle_quota.rb

@@ -31,7 +31,7 @@ Examples:
       end
 
       def command
-        prev_state = quotas_admin.switch_exceed_throttle_quota(false) ? 'true' : 'false'
+        prev_state = !!quotas_admin.switch_exceed_throttle_quota(false)
         formatter.row(["Previous exceed throttle quota enabled : #{prev_state}"])
         prev_state
       end

hbase-shell/src/main/ruby/shell/commands/disable_rpc_throttle.rb

@@ -31,7 +31,7 @@ Examples:
       end
 
       def command
-        prev_state = quotas_admin.switch_rpc_throttle(false) ? 'true' : 'false'
+        prev_state = !!quotas_admin.switch_rpc_throttle(false)
         formatter.row(["Previous rpc throttle state : #{prev_state}"])
         prev_state
       end

hbase-shell/src/main/ruby/shell/commands/enable_exceed_throttle_quota.rb

@@ -41,7 +41,7 @@ Examples:
       end
 
       def command
-        prev_state = quotas_admin.switch_exceed_throttle_quota(true) ? 'true' : 'false'
+        prev_state = !!quotas_admin.switch_exceed_throttle_quota(true)
         formatter.row(["Previous exceed throttle quota enabled : #{prev_state}"])
         prev_state
       end

hbase-shell/src/main/ruby/shell/commands/enable_rpc_throttle.rb

@@ -31,7 +31,7 @@ Examples:
       end
 
       def command
-        prev_state = quotas_admin.switch_rpc_throttle(true) ? 'true' : 'false'
+        prev_state = !!quotas_admin.switch_rpc_throttle(true)
         formatter.row(["Previous rpc throttle state : #{prev_state}"])
         prev_state
       end

hbase-shell/src/main/ruby/shell/commands/is_disabled.rb

@@ -29,8 +29,10 @@ EOF
       end
 
       def command(table)
-        formatter.row([admin.disabled?(table) ? 'true' : 'false'])
-    end
+        disabled = !!admin.disabled?(table)
+        formatter.row([disabled.to_s])
+        disabled
+      end
     end
   end
 end

hbase-shell/src/main/ruby/shell/commands/normalize.rb

@@ -33,7 +33,9 @@ EOF
       end
 
       def command
-        formatter.row([admin.normalize ? 'true' : 'false'])
+        did_normalize_run = !!admin.normalize
+        formatter.row([did_normalize_run.to_s])
+        did_normalize_run
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/normalizer_enabled.rb

@@ -30,7 +30,9 @@ EOF
       end
 
       def command
-        formatter.row([admin.normalizer_enabled?.to_s])
+        current_state = admin.normalizer_enabled?
+        formatter.row([current_state.to_s])
+        current_state
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/normalizer_switch.rb

@@ -32,7 +32,9 @@ EOF
       end
 
       def command(enableDisable)
-        formatter.row([admin.normalizer_switch(enableDisable) ? 'true' : 'false'])
+        previous_state = !!admin.normalizer_switch(enableDisable)
+        formatter.row([previous_state.to_s])
+        previous_state
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/snapshot_cleanup_switch.rb

@@ -34,7 +34,7 @@ Examples:
       end
 
       def command(enable_disable)
-        prev_state = admin.snapshot_cleanup_switch(enable_disable) ? 'true' : 'false'
+        prev_state = !!admin.snapshot_cleanup_switch(enable_disable)
         formatter.row(["Previous snapshot cleanup state : #{prev_state}"])
         prev_state
       end

hbase-shell/src/main/ruby/shell/commands/splitormerge_enabled.rb

@@ -30,9 +30,9 @@ EOF
       end
 
       def command(switch_type)
-        formatter.row(
-          [admin.splitormerge_enabled(switch_type) ? 'true' : 'false']
-        )
+        current_state = !!admin.splitormerge_enabled(switch_type)
+        formatter.row([current_state.to_s])
+        current_state
       end
     end
   end

hbase-shell/src/main/ruby/shell/commands/splitormerge_switch.rb

@@ -32,9 +32,11 @@ EOF
       end
 
       def command(switch_type, enabled)
+        previous_state = !!admin.splitormerge_switch(switch_type, enabled)
         formatter.row(
-          [admin.splitormerge_switch(switch_type, enabled) ? 'true' : 'false']
+          [previous_state.to_s]
         )
+        previous_state
       end
     end
   end

hbase-shell/src/test/ruby/hbase/admin_test.rb

@@ -104,8 +104,10 @@ module Hbase
     end
 
     define_test 'clear_deadservers should show exact row(s) count' do
-      output = capture_stdout { command(:clear_deadservers, 'test.server.com,16020,1574583397867') }
+      deadservers = []
+      output = capture_stdout { deadservers = command(:clear_deadservers, 'test.server.com,16020,1574583397867') }
       assert(output.include?('1 row(s)'))
+      assert(deadservers[0] == 'test.server.com,16020,1574583397867')
     end
 
     #-------------------------------------------------------------------------------
@@ -196,7 +198,8 @@ module Hbase
       output = capture_stdout { command(:balancer_enabled) }
       assert(output.include?('true'))
 
-      command(:balancer)
+      did_balancer_run = command(:balancer)
+      assert(did_balancer_run == true)
       output = capture_stdout { command(:balancer, 'force') }
       assert(output.include?('true'))
     end
@@ -212,13 +215,182 @@ module Hbase
     #-------------------------------------------------------------------------------
 
     define_test 'snapshot auto cleanup should work' do
-      command(:snapshot_cleanup_switch, true)
-      output = capture_stdout { command(:snapshot_cleanup_enabled) }
-      assert(output.include?('true'))
-
+      result = nil
       command(:snapshot_cleanup_switch, false)
-      output = capture_stdout { command(:snapshot_cleanup_enabled) }
+
+      # enable snapshot cleanup and check that the previous state is returned
+      output = capture_stdout { result = command(:snapshot_cleanup_switch, true) }
       assert(output.include?('false'))
+      assert(result == false)
+
+      # check that snapshot_cleanup_enabled returns the current state
+      output = capture_stdout { result = command(:snapshot_cleanup_enabled) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # disable snapshot cleanup and check that the previous state is returned
+      output = capture_stdout { result = command(:snapshot_cleanup_switch, false) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # check that snapshot_cleanup_enabled returns the current state
+      output = capture_stdout { result = command(:snapshot_cleanup_enabled) }
+      assert(output.include?('false'))
+      assert(result == false)
+    end
+
+    #-------------------------------------------------------------------------------
+
+    define_test 'balancer switch should work' do
+      result = nil
+      command(:balance_switch, false)
+
+      # enable balancer and check that the previous state is returned
+      output = capture_stdout { result = command(:balance_switch, true) }
+      assert(output.include?('false'))
+      assert(result == false)
+
+      # check that balancer_enabled returns the current state
+      output = capture_stdout { result = command(:balancer_enabled) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # disable balancer and check that the previous state is returned
+      output = capture_stdout { result = command(:balance_switch, false) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # check that balancer_enabled returns the current state
+      output = capture_stdout { result = command(:balancer_enabled) }
+      assert(output.include?('false'))
+      assert(result == false)
+    end
+
+    #-------------------------------------------------------------------------------
+
+    define_test 'normalizer switch should work' do
+      result = nil
+      command(:normalizer_switch, false)
+
+      # enable normalizer and check that the previous state is returned
+      output = capture_stdout { result = command(:normalizer_switch, true) }
+      assert(output.include?('false'))
+      assert(result == false)
+
+      # check that normalizer_enabled returns the current state
+      output = capture_stdout { result = command(:normalizer_enabled) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # disable normalizer and check that the previous state is returned
+      output = capture_stdout { result = command(:normalizer_switch, false) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # check that normalizer_enabled returns the current state
+      output = capture_stdout { result = command(:normalizer_enabled) }
+      assert(output.include?('false'))
+      assert(result == false)
+    end
+
+    #-------------------------------------------------------------------------------
+
+    define_test 'catalogjanitor switch should work' do
+      result = nil
+      command(:catalogjanitor_switch, false)
+
+      # enable catalogjanitor and check that the previous state is returned
+      output = capture_stdout { result = command(:catalogjanitor_switch, true) }
+      assert(output.include?('false'))
+      assert(result == false)
+
+      # check that catalogjanitor_enabled returns the current state
+      output = capture_stdout { result = command(:catalogjanitor_enabled) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # disable catalogjanitor and check that the previous state is returned
+      output = capture_stdout { result = command(:catalogjanitor_switch, false) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # check that catalogjanitor_enabled returns the current state
+      output = capture_stdout { result = command(:catalogjanitor_enabled) }
+      assert(output.include?('false'))
+      assert(result == false)
+    end
+
+    #-------------------------------------------------------------------------------
+
+    define_test 'cleaner_chore switch should work' do
+      result = nil
+      command(:cleaner_chore_switch, false)
+
+      # enable cleaner_chore and check that the previous state is returned
+      output = capture_stdout { result = command(:cleaner_chore_switch, true) }
+      assert(output.include?('false'))
+      assert(result == false)
+
+      # check that cleaner_chore_enabled returns the current state
+      output = capture_stdout { result = command(:cleaner_chore_enabled) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # disable cleaner_chore and check that the previous state is returned
+      output = capture_stdout { result = command(:cleaner_chore_switch, false) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # check that cleaner_chore_enabled returns the current state
+      output = capture_stdout { result = command(:cleaner_chore_enabled) }
+      assert(output.include?('false'))
+      assert(result == false)
+    end
+
+    #-------------------------------------------------------------------------------
+
+    define_test 'splitormerge switch should work' do
+      # Author's note: All the other feature switches in hbase-shell only toggle one feature. This command operates on
+      # both the "SPLIT" and "MERGE", so you will note that both code paths need coverage.
+      result = nil
+      command(:splitormerge_switch, 'SPLIT', false)
+      command(:splitormerge_switch, 'MERGE', true)
+
+      # flip switch and check that the previous state is returned
+      output = capture_stdout { result = command(:splitormerge_switch, 'SPLIT', true) }
+      assert(output.include?('false'))
+      assert(result == false)
+
+      output = capture_stdout { result = command(:splitormerge_switch, 'MERGE', false) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      # check that splitormerge_enabled returns the current state
+      output = capture_stdout { result = command(:splitormerge_enabled, 'SPLIT') }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      output = capture_stdout { result = command(:splitormerge_enabled, 'MERGE') }
+      assert(output.include?('false'))
+      assert(result == false)
+
+      # flip switch and check that the previous state is returned
+      output = capture_stdout { result = command(:splitormerge_switch, 'SPLIT', false) }
+      assert(output.include?('true'))
+      assert(result == true)
+
+      output = capture_stdout { result = command(:splitormerge_switch, 'MERGE', true) }
+      assert(output.include?('false'))
+      assert(result == false)
+
+      # check that splitormerge_enabled returns the current state
+      output = capture_stdout { result = command(:splitormerge_enabled, 'SPLIT') }
+      assert(output.include?('false'))
+      assert(result == false)
+
+      output = capture_stdout { result = command(:splitormerge_enabled, 'MERGE') }
+      assert(output.include?('true'))
+      assert(result == true)
     end
 
     #-------------------------------------------------------------------------------
@@ -473,7 +645,11 @@ module Hbase
       admin.disable_all(@regex)
       assert(command(:is_disabled, @t1))
       assert(command(:is_disabled, @t2))
+      assert(!command(:is_enabled, @t1))
+      assert(!command(:is_enabled, @t2))
       admin.enable_all(@regex)
+      assert(!command(:is_disabled, @t1))
+      assert(!command(:is_disabled, @t2))
       assert(command(:is_enabled, @t1))
       assert(command(:is_enabled, @t2))
       admin.disable_all(@regex)

hbase-shell/src/test/ruby/hbase/quotas_test.rb

@@ -245,11 +245,15 @@ module Hbase
     end
 
     define_test 'switch rpc throttle' do
-      output = capture_stdout { command(:disable_rpc_throttle) }
+      result = nil
+      output = capture_stdout { result = command(:disable_rpc_throttle) }
       assert(output.include?('Previous rpc throttle state : true'))
+      assert(result == true)
 
-      output = capture_stdout { command(:enable_rpc_throttle) }
+      result = nil
+      output = capture_stdout { result = command(:enable_rpc_throttle) }
       assert(output.include?('Previous rpc throttle state : false'))
+      assert(result == false)
     end
 
     define_test 'can set and remove region server quota' do
@@ -275,11 +279,17 @@ module Hbase
 
     define_test 'switch exceed throttle quota' do
       command(:set_quota, TYPE => THROTTLE, REGIONSERVER => 'all', LIMIT => '1CU/sec')
-      output = capture_stdout { command(:enable_exceed_throttle_quota) }
-      assert(output.include?('Previous exceed throttle quota enabled : false'))
 
-      output = capture_stdout { command(:disable_exceed_throttle_quota) }
+      result = nil
+      output = capture_stdout { result = command(:enable_exceed_throttle_quota) }
+      assert(output.include?('Previous exceed throttle quota enabled : false'))
+      assert(result == false)
+
+      result = nil
+      output = capture_stdout { result = command(:disable_exceed_throttle_quota) }
       assert(output.include?('Previous exceed throttle quota enabled : true'))
+      assert(result == true)
+
       command(:set_quota, TYPE => THROTTLE, REGIONSERVER => 'all', LIMIT => NONE)
     end