diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java index a354e40..a5a9a67 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java @@ -375,9 +375,18 @@ public class SimpleLoadBalancer extends BaseLoadBalancer { } // If we still have regions to dish out, assign underloaded to max - if (0 < regionsToMove.size()) { + /**HBASE-17969 + * When reaching this point, all regionserver should have at least + * min regions. If we still use serversByLoad's order(ordered by load + * and then severname, see {@link ServerAndLoad.compareTo()}). + * Then servers with smaller servername will have bigger possibility + * to get the remain regions. So here we need shuffle the regionservers + * to assign the remain regions. + */ + if(0 < regionsToMove.size()) { + List serversWithLessLoad = new ArrayList(); for (Map.Entry> server : - serversByLoad.entrySet()) { + serversByLoad.entrySet()) { int regionCount = server.getKey().getLoad(); BalanceInfo balanceInfo = serverBalanceInfo.get(server.getKey().getServerName()); if(balanceInfo != null) { @@ -386,8 +395,12 @@ public class SimpleLoadBalancer extends BaseLoadBalancer { if(regionCount >= max) { break; } - addRegionPlan(regionsToMove, fetchFromTail, - server.getKey().getServerName(), regionsToReturn); + serversWithLessLoad.add(server.getKey().getServerName()); + } + //shuffle the less loaded server + Collections.shuffle(serversWithLessLoad, RANDOM); + for(ServerName serverName : serversWithLessLoad) { + addRegionPlan(regionsToMove, fetchFromTail, serverName, regionsToReturn); if (regionsToMove.isEmpty()) { break; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/BalancerTestBase.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/BalancerTestBase.java index 5bd6606..21c4860 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/BalancerTestBase.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/BalancerTestBase.java @@ -393,9 +393,10 @@ public class BalancerTestBase { updateLoad(map, source, -1); ServerName destination = plan.getDestination(); updateLoad(map, destination, +1); - - servers.get(source).remove(plan.getRegionInfo()); - servers.get(destination).add(plan.getRegionInfo()); + if(servers != null) { + servers.get(source).remove(plan.getRegionInfo()); + servers.get(destination).add(plan.getRegionInfo()); + } } } result.clear(); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestDefaultLoadBalancer.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestDefaultLoadBalancer.java index 355fc9a..9e61ef7 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestDefaultLoadBalancer.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestDefaultLoadBalancer.java @@ -17,6 +17,8 @@ */ package org.apache.hadoop.hbase.master.balancer; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -25,11 +27,13 @@ import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HRegionInfo; +import org.apache.hadoop.hbase.protobuf.generated.HBaseProtos; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.master.LoadBalancer; import org.apache.hadoop.hbase.master.RegionPlan; import org.apache.hadoop.net.DNSToSwitchMapping; +import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -127,4 +131,68 @@ public class TestDefaultLoadBalancer extends BalancerTestBase { } } } + + /** + * A Test case for HBASE-17969 Balance by table using SimpleLoadBalancer could end up imbalance + * Say we have three RS named s1, s2, s3. + * A table named table1 with 3 regions distributes on these rs like this: + * s1 1 + * s2 1 + * s3 3 + * In average, each rs will have min=1, max=2 regions. So balancer will run. + * For s1 and s2, they have already have min=1 regions. Balancer won't do any operation on them. + * But for s3, it exceed max=3, so balancer will remove one region from s3, + * and choose one rs from s1, \s2 to move to. + * But s1 and s2 have the same load. So we need a random here. otherwise will be chosen since + * servername s1 < s2(alphabet order, sorted by ServerAndLoad's compareTo method). + * It is OK for table1 itself. But if every table in the cluster have similar situations like table1, + * then the load in the cluster will always be like s1 > s2 > s3. + * @throws Exception + */ + @Test (timeout=60000) + public void testACaseOfImbalanceBalanceByTable() throws Exception { + int tables = 10; + //three servers + ServerName s1 = ServerName.valueOf("s1", 60000, 0); + ServerName s2 = ServerName.valueOf("s2", 60000, 0); + ServerName s3 = ServerName.valueOf("s3", 60000, 0); + Map> RegionDistributeForOneTable = new HashMap<>(); + //one region for s1 + List regionForS1 = new ArrayList<>(); + regionForS1.addAll(randomRegions(1)); + RegionDistributeForOneTable.put(s1, regionForS1); + //one region for s2 + List regionForS2 = new ArrayList<>(); + regionForS2.addAll(randomRegions(1)); + RegionDistributeForOneTable.put(s2, regionForS2); + //three region for s2 + List regionForS3 = new ArrayList<>(); + regionForS3.addAll(randomRegions(3)); + RegionDistributeForOneTable.put(s3, regionForS3); + + List servers = convertToList(RegionDistributeForOneTable); + Map serverLoad = new HashMap<>(); + for(ServerAndLoad serverAndLoad : servers) { + serverLoad.put(serverAndLoad.getServerName(), new ServerAndLoad(serverAndLoad.getServerName(), 0)); + } + + //mock balance 10 tables in the cluster + for(int i = 0; i < tables; i++ ) { + + List plans = loadBalancer.balanceCluster(RegionDistributeForOneTable); + List newLoad = reconcile(servers, plans, null); + for(ServerAndLoad load : newLoad) { + ServerName server = load.getServerName(); + ServerAndLoad oldLoad = serverLoad.get(load.getServerName()); + serverLoad.put(server, new ServerAndLoad(server, oldLoad.getLoad() + load.getLoad())); + } + + } + ServerAndLoad loadForS1 = serverLoad.get(s1); + //Like said above, if random is not used, s1 will always have 20 regions of the 10 tables + Assert.assertNotEquals(20, loadForS1.getLoad()); + + + + } }