-
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-26649 Support meta replica LoadBalance mode for RegionLocator#g… #4078
Conversation
Hi @Apache9, please review backport as there are some difference between master and branch-2, thanks. |
🎊 +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.
Sorry I do not think this is ready to go, also for the master PR. Haven't realized we have added a rpc call in the getMetaScan method. We should try to find another better way to do this.
@@ -53,6 +54,8 @@ | |||
import org.slf4j.Logger; | |||
import org.slf4j.LoggerFactory; | |||
|
|||
import static org.apache.hadoop.hbase.client.RegionLocator.LOCATOR_META_REPLICAS_MODE; |
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.
Put static import on top
if (conn != null) { | ||
try { | ||
try (Table metaTable = getMetaHTable(conn)) { | ||
numOfReplicas = metaTable.getDescriptor().getRegionReplication(); |
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.
Ah, this is a bit painful... We used to expect getMetaScan to only contain memory operations but here we will have network io... And everytime we call this method it will generate a rpc calll...
Looking at the code above, in the async code we also have this and we even introduce a blocking rpc call. I haven't realized this when reviewing the PR for master, but obviously it is incorrect, we should not do blocking call in the async code. We should try to find a better way...
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.
Nice find! Let me back out the master changes.
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 reverted the commit to the master. You are right, the network call is bad. How about changing getMetaScan from a static method to a non-static one, so it can use a cached meta Replica count? Need to handle the case that meta replica count is changed dynamically.
…etAllRegionLocations() Signed-off-by: Duo Zhang <[email protected]>
012ee7f
to
b73b924
Compare
…etAllRegionLocations()
Signed-off-by: Duo Zhang [email protected]