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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
