HBASE-29070 Balancer cost function epsilon is imprecise (apache#6597)

Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
HubSpot · Jan 13, 2025 · 58b742b · 58b742b
1 parent 1e740bc
commit 58b742b
Show file tree

Hide file tree

Showing 16 changed files with 83 additions and 130 deletions.
diff --git a/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CostFunction.java b/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CostFunction.java
@@ -25,7 +25,9 @@
 @InterfaceAudience.Private
 abstract class CostFunction {
 
-  public static final double COST_EPSILON = 0.0001;
+  public static double getCostEpsilon(double cost) {
+    return Math.ulp(cost);
+  }
 
   private float multiplier = 0;
 
@@ -101,13 +103,14 @@ public void updateWeight(double[] weights) {
    * @return The scaled value.
    */
   protected static double scale(double min, double max, double value) {
+    double costEpsilon = getCostEpsilon(max);
     if (
-      max <= min || value <= min || Math.abs(max - min) <= COST_EPSILON
-        || Math.abs(value - min) <= COST_EPSILON
+      max <= min || value <= min || Math.abs(max - min) <= costEpsilon
+        || Math.abs(value - min) <= costEpsilon
     ) {
       return 0;
     }
-    if (max <= min || Math.abs(max - min) <= COST_EPSILON) {
+    if (max <= min || Math.abs(max - min) <= costEpsilon) {
       return 0;
     }
 

diff --git a/...alancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java b/...alancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
@@ -333,7 +333,8 @@ void updateMetricsSize(int size) {
 
   private boolean areSomeRegionReplicasColocated(BalancerClusterState c) {
     regionReplicaHostCostFunction.prepare(c);
-    return (Math.abs(regionReplicaHostCostFunction.cost()) > CostFunction.COST_EPSILON);
+    double cost = Math.abs(regionReplicaHostCostFunction.cost());
+    return cost > CostFunction.getCostEpsilon(cost);
   }
 
   private String getBalanceReason(double total, double sumMultiplier) {

diff --git a/hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/BalancerTestBase.java b/hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/BalancerTestBase.java
@@ -285,6 +285,9 @@ protected List<ServerAndLoad> convertToList(final Map<ServerName, List<RegionInf
   }
 
   protected String printMock(List<ServerAndLoad> balancedCluster) {
+    if (balancedCluster == null) {
+      return "null";
+    }
     NavigableSet<ServerAndLoad> sorted = new TreeSet<>(balancedCluster);
     ServerAndLoad[] arr = sorted.toArray(new ServerAndLoad[sorted.size()]);
     StringBuilder sb = new StringBuilder(sorted.size() * 4 + 4);

diff --git a/...cer/src/test/java/org/apache/hadoop/hbase/master/balancer/StochasticBalancerTestBase.java b/...cer/src/test/java/org/apache/hadoop/hbase/master/balancer/StochasticBalancerTestBase.java
@@ -17,9 +17,10 @@
  */
 package org.apache.hadoop.hbase.master.balancer;
 
-import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
+import java.time.Duration;
 import java.util.List;
 import java.util.Map;
 import org.apache.hadoop.hbase.HBaseConfiguration;
@@ -36,6 +37,7 @@
 public class StochasticBalancerTestBase extends BalancerTestBase {
 
   private static final Logger LOG = LoggerFactory.getLogger(StochasticBalancerTestBase.class);
+  private static final Duration MAX_MAX_RUN_TIME = Duration.ofSeconds(60);
 
   protected static StochasticLoadBalancer loadBalancer;
 
@@ -48,17 +50,15 @@ public static void beforeAllTests() throws Exception {
     conf.setClass("hbase.util.ip.to.rack.determiner", MockMapping.class, DNSToSwitchMapping.class);
     conf.setFloat("hbase.master.balancer.stochastic.localityCost", 0);
     conf.setBoolean("hbase.master.balancer.stochastic.runMaxSteps", true);
+    conf.setLong(StochasticLoadBalancer.MAX_RUNNING_TIME_KEY, 250);
     loadBalancer = new StochasticLoadBalancer(dummyMetricsStochasticBalancer);
     loadBalancer.setClusterInfoProvider(new DummyClusterInfoProvider(conf));
     loadBalancer.initialize();
   }
 
-  protected void testWithCluster(int numNodes, int numRegions, int numRegionsPerServer,
-    int replication, int numTables, boolean assertFullyBalanced,
-    boolean assertFullyBalancedForReplicas) {
-    Map<ServerName, List<RegionInfo>> serverMap =
-      createServerMap(numNodes, numRegions, numRegionsPerServer, replication, numTables);
-    testWithCluster(serverMap, null, assertFullyBalanced, assertFullyBalancedForReplicas);
+  protected void setMaxRunTime(Duration maxRunTime) {
+    conf.setLong(StochasticLoadBalancer.MAX_RUNNING_TIME_KEY, maxRunTime.toMillis());
+    loadBalancer.loadConf(conf);
   }
 
   protected void testWithClusterWithIteration(int numNodes, int numRegions, int numRegionsPerServer,
@@ -70,37 +70,14 @@ protected void testWithClusterWithIteration(int numNodes, int numRegions, int nu
       assertFullyBalancedForReplicas);
   }
 
-  protected void testWithCluster(Map<ServerName, List<RegionInfo>> serverMap,
-    RackManager rackManager, boolean assertFullyBalanced, boolean assertFullyBalancedForReplicas) {
-    List<ServerAndLoad> list = convertToList(serverMap);
-    LOG.info("Mock Cluster : " + printMock(list) + " " + printStats(list));
-
-    loadBalancer.setRackManager(rackManager);
-    // Run the balancer.
-    Map<TableName, Map<ServerName, List<RegionInfo>>> LoadOfAllTable =
-      (Map) mockClusterServersWithTables(serverMap);
-    List<RegionPlan> plans = loadBalancer.balanceCluster(LoadOfAllTable);
-    assertNotNull("Initial cluster balance should produce plans.", plans);
-
-    // Check to see that this actually got to a stable place.
-    if (assertFullyBalanced || assertFullyBalancedForReplicas) {
-      // Apply the plan to the mock cluster.
-      List<ServerAndLoad> balancedCluster = reconcile(list, plans, serverMap);
-
-      // Print out the cluster loads to make debugging easier.
-      LOG.info("Mock after Balance : " + printMock(balancedCluster));
-
-      if (assertFullyBalanced) {
-        assertClusterAsBalanced(balancedCluster);
-        LoadOfAllTable = (Map) mockClusterServersWithTables(serverMap);
-        List<RegionPlan> secondPlans = loadBalancer.balanceCluster(LoadOfAllTable);
-        assertNull("Given a requirement to be fully balanced, second attempt at plans should "
-          + "produce none.", secondPlans);
-      }
-
-      if (assertFullyBalancedForReplicas) {
-        assertRegionReplicaPlacement(serverMap, rackManager);
-      }
+  protected void increaseMaxRunTimeOrFail() {
+    Duration current = getCurrentMaxRunTime();
+    assertTrue(current.toMillis() < MAX_MAX_RUN_TIME.toMillis());
+    Duration newMax = Duration.ofMillis(current.toMillis() * 2);
+    if (newMax.toMillis() > MAX_MAX_RUN_TIME.toMillis()) {
+      setMaxRunTime(MAX_MAX_RUN_TIME);
+    } else {
+      setMaxRunTime(newMax);
     }
   }
 
@@ -114,7 +91,13 @@ protected void testWithClusterWithIteration(Map<ServerName, List<RegionInfo>> se
     Map<TableName, Map<ServerName, List<RegionInfo>>> LoadOfAllTable =
       (Map) mockClusterServersWithTables(serverMap);
     List<RegionPlan> plans = loadBalancer.balanceCluster(LoadOfAllTable);
-    assertNotNull("Initial cluster balance should produce plans.", plans);
+    if (plans == null) {
+      LOG.debug("First plans are null. Trying more balancer time, or will fail");
+      increaseMaxRunTimeOrFail();
+      testWithClusterWithIteration(serverMap, rackManager, assertFullyBalanced,
+        assertFullyBalancedForReplicas);
+      return;
+    }
 
     List<ServerAndLoad> balancedCluster = null;
     // Run through iteration until done. Otherwise will be killed as test time out
@@ -140,4 +123,9 @@ protected void testWithClusterWithIteration(Map<ServerName, List<RegionInfo>> se
       assertRegionReplicaPlacement(serverMap, rackManager);
     }
   }
+
+  private Duration getCurrentMaxRunTime() {
+    return Duration.ofMillis(conf.getLong(StochasticLoadBalancer.MAX_RUNNING_TIME_KEY,
+      StochasticLoadBalancer.DEFAULT_MAX_RUNNING_TIME));
+  }
 }
diff --git a/...ava/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerBalanceCluster.java b/...ava/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerBalanceCluster.java
@@ -19,6 +19,7 @@
 
 import static org.junit.Assert.assertNull;
 
+import java.time.Duration;
 import java.util.List;
 import java.util.Map;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
@@ -51,6 +52,7 @@ public class TestStochasticLoadBalancerBalanceCluster extends StochasticBalancer
    */
   @Test
   public void testBalanceCluster() throws Exception {
+    setMaxRunTime(Duration.ofMillis(1500));
     loadBalancer.onConfigurationChange(conf);
     for (int[] mockCluster : clusterStateMocks) {
       Map<ServerName, List<RegionInfo>> servers = mockClusterServers(mockCluster);

diff --git a/.../org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerHeterogeneousCost.java b/.../org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerHeterogeneousCost.java
@@ -19,7 +19,6 @@
 
 import static org.apache.hadoop.hbase.master.balancer.HeterogeneousCostRulesTestHelper.DEFAULT_RULES_FILE_NAME;
 import static org.apache.hadoop.hbase.master.balancer.HeterogeneousCostRulesTestHelper.createRulesFile;
-import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
@@ -40,16 +39,13 @@
 import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.client.RegionInfo;
 import org.apache.hadoop.hbase.client.RegionReplicaUtil;
-import org.apache.hadoop.hbase.master.RackManager;
 import org.apache.hadoop.hbase.master.RegionPlan;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 @Category({ MasterTests.class, MediumTests.class })
 public class TestStochasticLoadBalancerHeterogeneousCost extends StochasticBalancerTestBase {
@@ -58,9 +54,6 @@ public class TestStochasticLoadBalancerHeterogeneousCost extends StochasticBalan
   public static final HBaseClassTestRule CLASS_RULE =
     HBaseClassTestRule.forClass(TestStochasticLoadBalancerHeterogeneousCost.class);
 
-  private static final Logger LOG =
-    LoggerFactory.getLogger(TestStochasticLoadBalancerHeterogeneousCost.class);
-  private static final double ALLOWED_WINDOW = 1.20;
   private static final HBaseCommonTestingUtil HTU = new HBaseCommonTestingUtil();
   private static String RULES_FILE;
 
@@ -164,66 +157,7 @@ private void testHeterogeneousWithCluster(final int numNodes, final int numRegio
     createRulesFile(RULES_FILE, rules);
     final Map<ServerName, List<RegionInfo>> serverMap =
       this.createServerMap(numNodes, numRegions, numRegionsPerServer, 1, 1);
-    this.testWithCluster(serverMap, null, true, false);
-  }
-
-  @Override
-  protected void testWithCluster(final Map<ServerName, List<RegionInfo>> serverMap,
-    final RackManager rackManager, final boolean assertFullyBalanced,
-    final boolean assertFullyBalancedForReplicas) {
-    final List<ServerAndLoad> list = this.convertToList(serverMap);
-    LOG.info("Mock Cluster : " + this.printMock(list) + " " + this.printStats(list));
-
-    loadBalancer.setRackManager(rackManager);
-
-    // Run the balancer.
-    final List<RegionPlan> plans =
-      loadBalancer.balanceTable(HConstants.ENSEMBLE_TABLE_NAME, serverMap);
-    assertNotNull(plans);
-
-    // Check to see that this actually got to a stable place.
-    if (assertFullyBalanced || assertFullyBalancedForReplicas) {
-      // Apply the plan to the mock cluster.
-      final List<ServerAndLoad> balancedCluster = this.reconcile(list, plans, serverMap);
-
-      // Print out the cluster loads to make debugging easier.
-      LOG.info("Mock Balanced cluster : " + this.printMock(balancedCluster));
-
-      if (assertFullyBalanced) {
-        final List<RegionPlan> secondPlans =
-          loadBalancer.balanceTable(HConstants.ENSEMBLE_TABLE_NAME, serverMap);
-        assertNull(secondPlans);
-
-        // create external cost function to retrieve limit
-        // for each RS
-        final HeterogeneousRegionCountCostFunction cf =
-          new HeterogeneousRegionCountCostFunction(conf);
-        assertNotNull(cf);
-        BalancerClusterState cluster = new BalancerClusterState(serverMap, null, null, null);
-        cf.prepare(cluster);
-
-        // checking that we all hosts have a number of regions below their limit
-        for (final ServerAndLoad serverAndLoad : balancedCluster) {
-          final ServerName sn = serverAndLoad.getServerName();
-          final int numberRegions = serverAndLoad.getLoad();
-          final int limit = cf.findLimitForRS(sn);
-
-          double usage = (double) numberRegions / (double) limit;
-          LOG.debug(
-            sn.getHostname() + ":" + numberRegions + "/" + limit + "(" + (usage * 100) + "%)");
-
-          // as the balancer is stochastic, we cannot check exactly the result of the balancing,
-          // hence the allowedWindow parameter
-          assertTrue("Host " + sn.getHostname() + " should be below "
-            + cf.overallUsage * ALLOWED_WINDOW * 100 + "%; " + cf.overallUsage + ", " + usage + ", "
-            + numberRegions + ", " + limit, usage <= cf.overallUsage * ALLOWED_WINDOW);
-        }
-      }
-
-      if (assertFullyBalancedForReplicas) {
-        this.assertRegionReplicaPlacement(serverMap, rackManager);
-      }
-    }
+    this.testWithClusterWithIteration(serverMap, null, true, false);
   }
 
   @Override
@@ -306,6 +240,10 @@ static class StochasticLoadTestBalancer extends StochasticLoadBalancer {
     private FairRandomCandidateGenerator fairRandomCandidateGenerator =
       new FairRandomCandidateGenerator();
 
+    StochasticLoadTestBalancer() {
+      super(new DummyMetricsStochasticBalancer());
+    }
+
     @Override
     protected CandidateGenerator getRandomGenerator() {
       return fairRandomCandidateGenerator;

diff --git a/.../java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerLargeCluster.java b/.../java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerLargeCluster.java
@@ -17,6 +17,7 @@
  */
 package org.apache.hadoop.hbase.master.balancer;
 
+import java.time.Duration;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
@@ -38,7 +39,7 @@ public void testLargeCluster() {
     int numRegionsPerServer = 80; // all servers except one
     int numTables = 100;
     int replication = 1;
-    conf.setLong("hbase.master.balancer.stochastic.maxRunningTime", 6 * 60 * 1000);
+    setMaxRunTime(Duration.ofSeconds(30));
     loadBalancer.onConfigurationChange(conf);
     testWithClusterWithIteration(numNodes, numRegions, numRegionsPerServer, replication, numTables,
       true, true);

diff --git a/...st/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerMidCluster.java b/...st/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerMidCluster.java
@@ -17,6 +17,7 @@
  */
 package org.apache.hadoop.hbase.master.balancer;
 
+import java.time.Duration;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.testclassification.LargeTests;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
@@ -38,7 +39,8 @@ public void testMidCluster() {
     int numRegionsPerServer = 60; // all servers except one
     int replication = 1;
     int numTables = 40;
-    testWithCluster(numNodes, numRegions, numRegionsPerServer, replication, numTables, true, true);
+    testWithClusterWithIteration(numNodes, numRegions, numRegionsPerServer, replication, numTables,
+      true, true);
   }
 
   @Test
@@ -50,7 +52,8 @@ public void testMidCluster2() {
     int numTables = 400;
     // num large num regions means may not always get to best balance with one run
     boolean assertFullyBalanced = false;
-    testWithCluster(numNodes, numRegions, numRegionsPerServer, replication, numTables,
+    setMaxRunTime(Duration.ofMillis(2500));
+    testWithClusterWithIteration(numNodes, numRegions, numRegionsPerServer, replication, numTables,
       assertFullyBalanced, false);
   }
 
@@ -61,7 +64,8 @@ public void testMidCluster3() {
     int numRegionsPerServer = 9; // all servers except one
     int replication = 1;
     int numTables = 110;
-    testWithCluster(numNodes, numRegions, numRegionsPerServer, replication, numTables, true, true);
+    testWithClusterWithIteration(numNodes, numRegions, numRegionsPerServer, replication, numTables,
+      true, true);
     // TODO(eclark): Make sure that the tables are well distributed.
   }
 }
diff --git a/...java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerRegionReplica.java b/...java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerRegionReplica.java
@@ -171,7 +171,8 @@ public void testRegionReplicasOnSmallCluster() {
     int replication = 3; // 3 replicas per region
     int numRegionsPerServer = 80; // all regions are mostly balanced
     int numTables = 10;
-    testWithCluster(numNodes, numRegions, numRegionsPerServer, replication, numTables, true, true);
+    testWithClusterWithIteration(numNodes, numRegions, numRegionsPerServer, replication, numTables,
+      true, true);
   }
 
   private static class ForTestRackManagerOne extends RackManager {

diff --git a/.../hadoop/hbase/master/balancer/TestStochasticLoadBalancerRegionReplicaHighReplication.java b/.../hadoop/hbase/master/balancer/TestStochasticLoadBalancerRegionReplicaHighReplication.java
@@ -17,6 +17,7 @@
  */
 package org.apache.hadoop.hbase.master.balancer;
 
+import java.time.Duration;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
@@ -35,13 +36,14 @@ public class TestStochasticLoadBalancerRegionReplicaHighReplication
   @Test
   public void testRegionReplicasOnMidClusterHighReplication() {
     conf.setLong(StochasticLoadBalancer.MAX_STEPS_KEY, 4000000L);
-    conf.setLong("hbase.master.balancer.stochastic.maxRunningTime", 120 * 1000); // 120 sec
+    setMaxRunTime(Duration.ofSeconds(5));
     loadBalancer.onConfigurationChange(conf);
     int numNodes = 40;
     int numRegions = 6 * numNodes;
     int replication = 40; // 40 replicas per region, one for each server
     int numRegionsPerServer = 5;
     int numTables = 10;
-    testWithCluster(numNodes, numRegions, numRegionsPerServer, replication, numTables, false, true);
+    testWithClusterWithIteration(numNodes, numRegions, numRegionsPerServer, replication, numTables,
+      false, true);
   }
 }