HBASE-29072 StochasticLoadBalancer#areReplicasColocated ignores rack colocation #6622
Conversation
| int numReplicas = region.getReplicaId() + 1; | ||
| if (numReplicas > maxReplicas) { | ||
| maxReplicas = numReplicas; | ||
| } |
There was a problem hiding this comment.
This seemed like a very cheap way to determine the max replica count without actually seeing the table descriptors, which is a limitation of the current balancer implementation.
Sometime I would like to fix this limitation and make the balancer table descriptor aware, but I think that is its own problem
There was a problem hiding this comment.
I'm pretty sure that you have insufficient information here. I'm looking at the call hierarchy of registerRegion and I believe that there's no limit by table. So you're approximating the maximum number of region replicas used by any table in the cluster? I suspect that you need to introduce a new index by table name to the number of replicas per table.
There was a problem hiding this comment.
Agreed, we could make this better with table specific replica count tracking
There was a problem hiding this comment.
Okay but that additional tracking is not a correctness issue for how this value is used?
There was a problem hiding this comment.
The BalancerClusterState is only aware of the regions for tables that it is actively trying to balance — so if you're balancing by table (not the default) then I believe table specific tracking will not make this any better. If you're balancing by cluster (the default) then I believe table specific tracking could maybe make things better by checking for rack replica colocation by table... actually now that I think about it the cost functions probably don't support this. So, tldr, if you're doing cluster-wide balancing then this will cause us to ignore rack colocation as a trigger once any single table's replica counts are greater than the number of racks, and the way to get more specific decisions would be to enable byTable balancing
| assertFalse(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME, | ||
| new BalancerClusterState(map, null, null, new ForTestRackManagerOne()))); |
There was a problem hiding this comment.
In that PR where we made the balancer ignorant to rack colocation, we also just naively switched this assertion to false. But the intention of this test was always for the assertion to be true.
I've also split this test up into two tests, because it was always testing two distinct cases (host and rack colocation).
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
left a comment
ndimiduk
left a comment
There was a problem hiding this comment.
Nice one Ray. See my question about inferring maxReplicas and how we maybe aught-should track it. Otherwise, LGTM.
| int numReplicas = region.getReplicaId() + 1; | ||
| if (numReplicas > maxReplicas) { | ||
| maxReplicas = numReplicas; | ||
| } |
There was a problem hiding this comment.
I'm pretty sure that you have insufficient information here. I'm looking at the call hierarchy of registerRegion and I believe that there's no limit by table. So you're approximating the maximum number of region replicas used by any table in the cluster? I suspect that you need to introduce a new index by table name to the number of replicas per table.
...e-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
Show resolved
Hide resolved
…colocation (#6622) Co-authored-by: Ray Mattingly <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>
…colocation (#6622) Co-authored-by: Ray Mattingly <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]>
…colocation (#6622) (#6639) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
…colocation (#6622) (#6640) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
…colocation (#6622) (#6640) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
…colocation (apache#6622) (apache#6640) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
…ated ignores rack colocation (apache#6622) (apache#6640) (will be in 2.6.3) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
…ated ignores rack colocation (apache#6622) (apache#6640) (will be in 2.6.3) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
…colocation (#6622) (#6640) (#6644) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
…ated ignores rack colocation (apache#6622) (apache#6640) (will be in 2.6.3) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
…colocation (apache#6622) (apache#6640) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Ray Mattingly <[email protected]>
3 participants
This is yet another bite of #6593
In #3729 we removed consideration replicas' rack distribution. This was, in my opinion, a mistake — if we want to be flexible for environments that have too few racks, then we should just do so by skipping this check when the rack count is < the max replica count