Backport "HBASE-29072 StochasticLoadBalancer#areReplicasColocated ignores rack …" to branch-3

hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerClusterState.java

@@ -106,6 +106,7 @@ class BalancerClusterState {
   int numRacks;
   int numTables;
   int numRegions;
+  int maxReplicas = 1;
 
   int numMovedRegions = 0; // num moved regions from the initial configuration
   Map<ServerName, List<RegionInfo>> clusterState;
@@ -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;
+    }
   }
 
   /**

hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java

@@ -342,10 +342,33 @@ void updateMetricsSize(int size) {
     }
   }
 
-  private boolean areSomeRegionReplicasColocated(BalancerClusterState c) {
-    regionReplicaHostCostFunction.prepare(c);
-    double cost = Math.abs(regionReplicaHostCostFunction.cost());
-    return cost > CostFunction.getCostEpsilon(cost);
+  private boolean areSomeRegionReplicasColocatedOnHost(BalancerClusterState c) {
+    if (c.numHosts >= c.maxReplicas) {
+      regionReplicaHostCostFunction.prepare(c);
+      double hostCost = Math.abs(regionReplicaHostCostFunction.cost());
+      boolean colocatedAtHost = hostCost > CostFunction.getCostEpsilon(hostCost);
+      if (colocatedAtHost) {
+        return true;
+      }
+      LOG.trace("No host colocation detected with host cost={}", hostCost);
+    }
+    return false;
+  }
+
+  private boolean areSomeRegionReplicasColocatedOnRack(BalancerClusterState c) {
+    if (c.numRacks >= c.maxReplicas) {
+      regionReplicaRackCostFunction.prepare(c);
+      double rackCost = Math.abs(regionReplicaRackCostFunction.cost());
+      boolean colocatedAtRack = rackCost > CostFunction.getCostEpsilon(rackCost);
+      if (colocatedAtRack) {
+        return true;
+      }
+      LOG.trace("No rack colocation detected with rack cost={}", rackCost);
+    } else {
+      LOG.trace("Rack colocation is inevitable with fewer racks than replicas, "
+        + "so we won't bother checking");
+    }
+    return false;
   }
 
   private String getBalanceReason(double total, double sumMultiplier) {
@@ -372,12 +395,19 @@ boolean needsBalance(TableName tableName, BalancerClusterState cluster) {
         + " < MIN_SERVER_BALANCE(" + MIN_SERVER_BALANCE + ")", null);
       return false;
     }
-    if (areSomeRegionReplicasColocated(cluster)) {
+
+    if (areSomeRegionReplicasColocatedOnHost(cluster)) {
       LOG.info("Running balancer because at least one server hosts replicas of the same region."
         + " function cost={}", functionCost());
       return true;
     }
 
+    if (areSomeRegionReplicasColocatedOnRack(cluster)) {
+      LOG.info("Running balancer because at least one rack hosts replicas of the same region."
+        + " function cost={}", functionCost());
+      return true;
+    }
+
     if (idleRegionServerExist(cluster)) {
       LOG.info("Running balancer because cluster has idle server(s)." + " function cost={}",
         functionCost());

hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerRegionReplica.java

@@ -41,6 +41,8 @@
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableList;
+
 @Category({ MasterTests.class, LargeTests.class })
 public class TestStochasticLoadBalancerRegionReplica extends StochasticBalancerTestBase {
 
@@ -136,7 +138,7 @@ public void testReplicaCostForReplicas() {
   }
 
   @Test
-  public void testNeedsBalanceForColocatedReplicas() {
+  public void testNeedsBalanceForColocatedReplicasOnHost() {
     // check for the case where there are two hosts and with one rack, and where
     // both the replicas are hosted on the same server
     List<RegionInfo> regions = randomRegions(1);
@@ -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())));
+    BalancerClusterState cluster =
+      new BalancerClusterState(map, null, null, new ForTestRackManagerOne());
+    loadBalancer.initCosts(cluster);
+    assertTrue(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME, cluster));
+  }
+
+  @Test
+  public void testNoNeededBalanceForColocatedReplicasTooFewRacks() {
+    // Three hosts, two racks, and three replicas for a region. This cannot be balanced
+    List<RegionInfo> regions = randomRegions(1);
+    ServerName s1 = ServerName.valueOf("host1", 1000, 11111);
+    ServerName s2 = ServerName.valueOf("host11", 1000, 11111);
+    ServerName s3 = ServerName.valueOf("host2", 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);
+    // there are 3 replicas for region 0, but only add a second rack
+    map.put(s3, ImmutableList.of(RegionReplicaUtil.getRegionInfoForReplica(regions.get(0), 2)));
+    BalancerClusterState cluster =
+      new BalancerClusterState(map, null, null, new ForTestRackManagerOne());
+    loadBalancer.initCosts(cluster);
+    // Should be false because there aren't enough racks
+    assertFalse(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME, cluster));
   }
 
   @Test