[GitHub] [hbase] d-c-manning commented on a change in pull request #3972: HBASE-26596 region_mover should gracefully ignore null response from RSGroupAdmin#getRSGroupOfServer

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

d-c-manning commented on a change in pull request #3972:
URL: https://github.com/apache/hbase/pull/3972#discussion_r774737883



##########
File path: bin/region_mover.rb
##########
@@ -493,11 +493,22 @@ def getFilename(options, targetServer, port)
   return filename
 end
 
+def getServersRSGroup(rsgroup_admin, hostname, port)
+  rsgroup = rsgroup_admin.getRSGroupOfServer(Address.fromParts(hostname,
+                                             java.lang.Integer.parseInt(port)))
+  # If the rsgroup is nil, that means this server belongs to no rsgroup.
+  # It should be already offline.
+  # Here we directly log and exit.
+  return rsgroup unless rsgroup.nil?
+  $LOG.info('The server ' + hostname + 'belongs to no rsgroup. Exit regions moving.')
+  exit 0

Review comment:
       The code and syntax looks good to me
   >Here, I see one minor issue: if we go with this change, still the unload region would not make a graceful exit:
   Can we instead call rsgroupAdmin.getRSGroupOfServer outside of getSameRSGroupServers method and if it returns nil, we can log a message and then exit gracefully?
   
   I do question this design decision, though. If a server has regions on it, but fails to find any target servers to unload to, then that seems like a reason to exit with an error code.
   
   So then I would propose that we explicitly check for this case. It's okay to exit with 0 if the server has no rsgroup AND has no regions. This would obviously require additional refactoring of the code, since currently we don't check for regions until after we obtain the rsgroup of the server. And in the case mentioned in HBASE-26596, I don't know whether a call to `getOnlineRegions` would succeed.
   
   This is only my opinion, so feel free to ignore if you feel differently 🙂 

##########
File path: bin/region_mover.rb
##########
@@ -493,11 +493,22 @@ def getFilename(options, targetServer, port)
   return filename
 end
 
+def getServersRSGroup(rsgroup_admin, hostname, port)
+  rsgroup = rsgroup_admin.getRSGroupOfServer(Address.fromParts(hostname,
+                                             java.lang.Integer.parseInt(port)))
+  # If the rsgroup is nil, that means this server belongs to no rsgroup.
+  # It should be already offline.
+  # Here we directly log and exit.
+  return rsgroup unless rsgroup.nil?
+  $LOG.info('The server ' + hostname + 'belongs to no rsgroup. Exit regions moving.')

Review comment:
       This feels like it justifies at least a `warn` level log




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