-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-25365 The log in move_servers_rsgroup is incorrect #2742
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor feedback. Looks good otherwise.
@@ -1015,17 +1016,17 @@ private void moveServerRegionsFromGroup(Set<Address> movedServers, Set<Address> | |||
assignmentFutures.add(Pair.newPair(region, future)); | |||
} catch (IOException ioe) { | |||
failedRegions.add(region.getRegionNameAsString()); | |||
LOG.debug("Move region {} from group failed, will retry, current retry time is {}", | |||
region.getShortNameToLog(), retry, ioe); | |||
LOG.debug("Move region {} from server {} failed, will retry, current retry time is {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 'server' in the log message is redundant; we are always moving between servers?
if (failedRegions.isEmpty()) { | ||
LOG.info("All regions from server(s) {} moved to target group {}.", movedServerNames, | ||
targetGrp.getName()); | ||
LOG.info("All regions from server(s) {} moved to source group {}.", movedServerNames, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not say, 'All regions from {} moved from {} to {}, movedServerNames, sourceGroupName, targetGroupName?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regions in movedServerNames originally belongs to the source group. So it should not say "move from sourceGroupName to targetGroupName". Maybe it can be like this: “All regions from {} are moved back to {}, movedServerNames, sourceGroupName”?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is what is happening, then yes, your rephrasing makes more sense (IMO). Thanks.
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
HBASE-25365