-
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-26885 The TRSP should not go on when it get a bogus server name… #4276
Conversation
🎊 +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.
Left minor comment, looks good otherwise
@@ -199,7 +199,7 @@ private void queueAssign(MasterProcedureEnv env, RegionStateNode regionNode) | |||
|
|||
private void openRegion(MasterProcedureEnv env, RegionStateNode regionNode) throws IOException { | |||
ServerName loc = regionNode.getRegionLocation(); | |||
if (loc == null) { | |||
if (loc == null || env.getMasterServices().getLoadBalancer().isBogusServerName(loc)) { |
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.
I think we should directly check with BOGUS_SERVER_NAME.equals(loc)
here. BOGUS_SERVER_NAME
is anyways public.
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.
Good point, will fix, thanks.
* Check if the ServerName is bogus. | ||
* @param sn server name to be checked | ||
*/ | ||
default boolean isBogusServerName(ServerName sn) { |
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.
Can be removed once we use BOGUS_SERVER_NAME
directly in TRSP
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.
Ditto.
I can not recall when will we return BOGUS_SERVER_NAME server name to upper layer... Could you please post your findings here? @bsglz And I agree with @virajjasani that let's use BOGUS_SERVER_NAME directly instead of adding a new method in LoadBalancer, it is unnecessary. Thanks. |
One case i find: all of the servers in a rsgroup were down. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +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.
It seems difficult to provide a UT, but if possible by some tricks, would be great :)
(even in follow-up Jira, it's fine)
Yeah, a bit difficult, will consider it later if i have time, thanks for the review. |
#4276) * HBASE-26885 The TRSP should not go on when it get a bogus server name from AM
#4276) * HBASE-26885 The TRSP should not go on when it get a bogus server name from AM (cherry picked from commit 1efd8fe)
#4276) * HBASE-26885 The TRSP should not go on when it get a bogus server name from AM (cherry picked from commit 1efd8fe)
apache#4276) * HBASE-26885 The TRSP should not go on when it get a bogus server name from AM (cherry picked from commit 1efd8fe) (cherry picked from commit a25e779) Change-Id: Ida2312101866595b3b68bfe6b0590b0f8d713014
… from AM