[GitHub] [hbase] saintstack commented on a change in pull request #2058: HBASE-24722 Address hbase-shell commands with unintentional return values

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

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



##########
File path: hbase-shell/src/test/ruby/hbase/admin_test.rb
##########
@@ -212,13 +215,182 @@ def teardown
     #-------------------------------------------------------------------------------
 
     define_test 'snapshot auto cleanup should work' do
-      command(:snapshot_cleanup_switch, true)
-      output = capture_stdout { command(:snapshot_cleanup_enabled) }
+      result = nil
+      command(:snapshot_cleanup_switch, false)
+
+      # 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)
 
-      command(:snapshot_cleanup_switch, false)
-      output = capture_stdout { command(:snapshot_cleanup_enabled) }
+      # 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)

Review comment:
       Sweet

##########
File path: hbase-shell/src/main/ruby/shell/commands/balance_switch.rb
##########
@@ -31,7 +31,7 @@ def help
       end
 
       def command(enableDisable)
-        prev_state = admin.balance_switch(enableDisable) ? 'true' : 'false'
+        prev_state = !!admin.balance_switch(enableDisable)

Review comment:
       Nice idiom convertion (java to ruby)

##########
File path: hbase-shell/src/main/ruby/shell/commands/clear_block_cache.rb
##########
@@ -33,6 +33,7 @@ def help
 
       def command(table_name)
         formatter.row([admin.clear_block_cache(table_name)])
+        nil

Review comment:
       formatter.row returns void? Different to nil?

##########
File path: hbase-shell/src/main/ruby/shell/commands/clear_deadservers.rb
##########
@@ -35,18 +37,20 @@ def help
       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 }

Review comment:
       Bug fix?




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