-
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-26327 Replicas cohosted on a rack shouldn't keep triggering Bal… #3729
Conversation
💔 -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. |
🎊 +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. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
The problem has been described nicely on jira, but looking at the code, I haven't gotten the point on how to fix the problem... Mind explaining a bit about the new algorithm? And on reservoir sampling, I've implemented a util class recently, it is Thanks. |
Sorry I think I mixed up the commits for HBASE-26309 and HBASE-26327 on this branch. Please review on the other PR for the algorithm. #3723. The summary of problem and fixes is posted at https://docs.google.com/document/d/1ELcFlXkF4q7x-ZfPd9vGkz8RCSw2oRbL-yMp8ANW2Rs/edit#heading=h.v8fhroxb12gn I will take a look at the utility too. For this PR, I will clean up the branch and push again. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
return true; | ||
} | ||
return false; | ||
return (Math.abs(regionReplicaHostCostFunction.cost()) > CostFunction.COST_EPSILON); |
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.
We no longer force balancer to run if there are still cohosted replicas on racks:
- there are cases it cannot be satisfied
- there are cases we have to accept it as a compromise to have better region count distribution.
💔 -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. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -161,8 +161,7 @@ public void testNeedsBalanceForColocatedReplicas() { | |||
map.put(s2, regionsOnS2); | |||
// add another server so that the cluster has some host on another rack | |||
map.put(ServerName.valueOf("host2", 1000, 11111), randomRegions(1)); | |||
assertFalse( | |||
loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME, | |||
assertTrue(!loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME, |
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.
Does it make any difference with the change?
assertFalse(func()) <==> assertTrue(!func())
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.
will update to assertFalse
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.
Looks good to me. Not sure if we need to add a config option saying that ignore Rack for replica balancer. I assume that we are good for now.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
If the weight is set to 0, it won't be considered for evaluation. However the 1 out 4 generator will still waste time generating candidates for rack balance. Therefor I have #3732. Your review would be helpful too. @huaxiangsun |
Thanks @clarax, let me review the PR. |
…ancer