rmdmattingly commented on code in PR #6622:
URL: https://github.com/apache/hbase/pull/6622#discussion_r1924227779
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerClusterState.java:
##########
@@ -446,6 +447,11 @@ private void registerRegion(RegionInfo region, int
regionIndex, int serverIndex,
: serversToIndex.get(loc.get(i).getAddress()));
}
}
+
+ int numReplicas = region.getReplicaId() + 1;
+ if (numReplicas > maxReplicas) {
+ maxReplicas = numReplicas;
+ }
Review 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
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerRegionReplica.java:
##########
@@ -148,20 +150,50 @@ public void testNeedsBalanceForColocatedReplicas() {
// until the step above s1 holds two replicas of a region
regions = randomRegions(1);
map.put(s2, regions);
- assertTrue(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME,
- new BalancerClusterState(map, null, null, null)));
- // check for the case where there are two hosts on the same rack and there
are two racks
- // and both the replicas are on the same rack
- map.clear();
- regions = randomRegions(1);
+ BalancerClusterState cluster =
+ new BalancerClusterState(map, null, null, new ForTestRackManagerOne());
+ loadBalancer.initCosts(cluster);
+ assertTrue(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME,
cluster));
+ }
+
+ @Test
+ public void testNeedsBalanceForColocatedReplicasOnRack() {
+ // Three hosts, two racks, and two replicas for a region. This should be
balanced
+ List<RegionInfo> regions = randomRegions(1);
+ ServerName s1 = ServerName.valueOf("host1", 1000, 11111);
+ ServerName s2 = ServerName.valueOf("host11", 1000, 11111);
+ Map<ServerName, List<RegionInfo>> map = new HashMap<>();
List<RegionInfo> regionsOnS2 = new ArrayList<>(1);
regionsOnS2.add(RegionReplicaUtil.getRegionInfoForReplica(regions.get(0),
1));
map.put(s1, regions);
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,
- new BalancerClusterState(map, null, null, new ForTestRackManagerOne())));
Review 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
