From a69c23abfeed9246f308bf470b72a9a8afa46f5d Mon Sep 17 00:00:00 2001 From: tedyu Date: Thu, 16 Mar 2017 19:07:59 -0700 Subject: [PATCH] HBASE-17706 TableSkewCostFunction improperly computes max skew - revert due to test failure --- .../master/balancer/BaseLoadBalancer.java | 19 +++++++------ .../balancer/StochasticLoadBalancer.java | 19 ++++++------- .../balancer/TestStochasticLoadBalancer.java | 28 ------------------- 3 files changed, 19 insertions(+), 47 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java index c6086f6875e..b0e088c0400 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java @@ -739,15 +739,18 @@ public abstract class BaseLoadBalancer implements LoadBalancer { } numRegionsPerServerPerTable[newServer][tableIndex]++; - // if old server had max num regions, assume (for now) - // max num regions went down since we moved the region - if (oldServer >= 0 && - (numRegionsPerServerPerTable[oldServer][tableIndex] + 1) == numMaxRegionsPerTable[tableIndex]) { - numMaxRegionsPerTable[tableIndex]--; + //check whether this caused maxRegionsPerTable in the new Server to be updated + if (numRegionsPerServerPerTable[newServer][tableIndex] > numMaxRegionsPerTable[tableIndex]) { + numMaxRegionsPerTable[tableIndex] = numRegionsPerServerPerTable[newServer][tableIndex]; + } else if (oldServer >= 0 && (numRegionsPerServerPerTable[oldServer][tableIndex] + 1) + == numMaxRegionsPerTable[tableIndex]) { + //recompute maxRegionsPerTable since the previous value was coming from the old server + for (int serverIndex = 0 ; serverIndex < numRegionsPerServerPerTable.length; serverIndex++) { + if (numRegionsPerServerPerTable[serverIndex][tableIndex] > numMaxRegionsPerTable[tableIndex]) { + numMaxRegionsPerTable[tableIndex] = numRegionsPerServerPerTable[serverIndex][tableIndex]; + } + } } - // Now check if new server sets new max - numMaxRegionsPerTable[tableIndex] = - Math.max(numMaxRegionsPerTable[tableIndex], numRegionsPerServerPerTable[newServer][tableIndex]); // update for servers int primary = regionIndexToPrimaryIndex[region]; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java index 2a3582ecefc..8cbdd1e9c67 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java @@ -272,6 +272,14 @@ public class StochasticLoadBalancer extends BaseLoadBalancer { @Override protected boolean needsBalance(Cluster cluster) { + ClusterLoadState cs = new ClusterLoadState(cluster.clusterState); + if (cs.getNumServers() < MIN_SERVER_BALANCE) { + if (LOG.isDebugEnabled()) { + LOG.debug("Not running balancer because only " + cs.getNumServers() + + " active regionserver(s)"); + } + return false; + } if (areSomeRegionReplicasColocated(cluster)) { return true; } @@ -298,17 +306,6 @@ public class StochasticLoadBalancer extends BaseLoadBalancer { + minCostNeedBalance); return false; } - - ClusterLoadState cs = new ClusterLoadState(cluster.clusterState); - if (cs.getNumServers() < MIN_SERVER_BALANCE) { - if (LOG.isDebugEnabled()) { - LOG.debug("Not running balancer because only " + cs.getNumServers() - + " active regionserver(s)"); - } - return false; - } - - return true; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java index 081db762a73..37ff35fdbeb 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java @@ -50,7 +50,6 @@ import org.apache.hadoop.hbase.master.RegionPlan; import org.apache.hadoop.hbase.master.balancer.BaseLoadBalancer.Cluster; import org.apache.hadoop.hbase.master.balancer.StochasticLoadBalancer.CandidateGenerator; import org.apache.hadoop.hbase.master.balancer.StochasticLoadBalancer.TableSkewCandidateGenerator; -import org.apache.hadoop.hbase.master.balancer.StochasticLoadBalancer.LoadCandidateGenerator; import org.apache.hadoop.hbase.testclassification.FlakeyTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; @@ -238,33 +237,6 @@ public class TestStochasticLoadBalancer extends BalancerTestBase { } } - @Test - public void testTableSkewCostProperlyDecreases() { - int replication = 1; - Configuration conf = HBaseConfiguration.create(); - StochasticLoadBalancer.CostFunction - costFunction = new StochasticLoadBalancer.TableSkewCostFunction(conf); - CandidateGenerator generator = new LoadCandidateGenerator(); - // Start out with 100 regions on one server and 0 regions on the other - int numNodes = 2; - int numTables = 1; - int numRegions = 100; - int numRegionsPerServer = 0; - - Map> serverMap = createServerMap(numNodes, numRegions, numRegionsPerServer, replication, numTables); - BaseLoadBalancer.Cluster cluster = new Cluster(serverMap, null, null, null); - costFunction.init(cluster); - double cost = costFunction.cost(); - assertEquals(1.0, cost, .0001); - for (int i = 0; i < 100; i++) { - Cluster.Action action = generator.generate(cluster); - cluster.doAction(action); - costFunction.postAction(action); - cost = costFunction.cost(); - } - assertTrue(cost < 0.5); - } - @Test public void testRegionLoadCost() { List regionLoads = new ArrayList<>();