Index: src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java (revision 1294673) +++ src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java (working copy) @@ -171,7 +171,7 @@ clusterState.entrySet()) { server.getKey().getLoad().setNumberOfRegions(server.getValue().size()); numRegions += server.getKey().getLoad().getNumberOfRegions(); - serversByLoad.put(server.getKey(), server.getValue()); + serversByLoad.put(server.getKey(), randomize(server.getValue())); strBalanceParam.append(server.getKey().getServerName()).append("=") .append(server.getValue().size()).append(", "); @@ -222,7 +222,7 @@ break; } serversOverloaded++; - List regions = randomize(server.getValue()); + List regions = server.getValue(); int numToOffload = Math.min(regionCount - max, regions.size()); int numTaken = 0; @@ -278,19 +278,32 @@ // If we need more to fill min, grab one from each most loaded until enough if (neededRegions != 0) { // Walk down most loaded, grabbing one from each until we get enough - for(Map.Entry> server : + for(Map.Entry> server : serversByLoad.descendingMap().entrySet()) { + // If region count less than or equal to min don't + // grab region from this server + if (server.getValue().size() <= min) break; BalanceInfo balanceInfo = serverBalanceInfo.get(server.getKey()); int idx = balanceInfo == null ? 0 : balanceInfo.getNextRegionForUnload(); if (idx >= server.getValue().size()) break; HRegionInfo region = server.getValue().get(idx); - if (region.isMetaRegion()) continue; // Don't move meta regions. - regionsToMove.add(new RegionPlan(region, server.getKey(), null)); - if(--neededRegions == 0) { - // No more regions needed, done shedding - break; + // If the idx position region is META or ROOT get the next region + // if it is available. + while (region != null && region.isMetaRegion()) { + region = null; + idx++; + if (idx >= server.getValue().size()) + break; + region = server.getValue().get(idx); } + if (region != null) { + regionsToMove.add(new RegionPlan(region, server.getKey(), null)); + if (--neededRegions == 0) { + // No more regions needed, done shedding + break; + } + } } } @@ -364,7 +377,7 @@ * @param regions * @return Randomization of passed regions */ - static List randomize(final List regions) { + List randomize(final List regions) { Collections.shuffle(regions, RANDOM); return regions; } Index: src/test/java/org/apache/hadoop/hbase/master/TestLoadBalancer.java =================================================================== --- src/test/java/org/apache/hadoop/hbase/master/TestLoadBalancer.java (revision 1294673) +++ src/test/java/org/apache/hadoop/hbase/master/TestLoadBalancer.java (working copy) @@ -25,6 +25,8 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -55,6 +57,8 @@ private static LoadBalancer loadBalancer; + public static List metaRootRegions; + private static Random rand; @BeforeClass @@ -63,6 +67,9 @@ conf.set("hbase.regions.slop", "0"); loadBalancer = new LoadBalancer(conf); rand = new Random(); + metaRootRegions = new ArrayList(); + metaRootRegions.add(HRegionInfo.ROOT_REGIONINFO); + metaRootRegions.add(HRegionInfo.FIRST_META_REGIONINFO); } // int[testnum][servernumber] -> numregions @@ -145,8 +152,8 @@ public void testRandomizer() { for(int [] mockCluster : clusterStateMocks) { if (mockCluster.length < 5) continue; - Map> servers = - mockClusterServers(mockCluster); + Map> servers = mockClusterServers( + mockCluster, false); for (Map.Entry> e: servers.entrySet()) { List original = e.getValue(); if (original.size() < 5) continue; @@ -159,7 +166,7 @@ for (HRegionInfo hri: copy) { System.out.println(hri.getEncodedName()); } - List randomized = LoadBalancer.randomize(copy); + List randomized = loadBalancer.randomize(copy); System.out.println("Randomizing after " + randomized.size()); for (HRegionInfo hri: randomized) { System.out.println(hri.getEncodedName()); @@ -183,21 +190,76 @@ */ @Test public void testBalanceCluster() throws Exception { + for (int[] mockCluster : clusterStateMocks) { + balanceCluster(mockCluster, false, null); + } + } - for(int [] mockCluster : clusterStateMocks) { - Map> servers = mockClusterServers(mockCluster); - LOG.info("Mock Cluster : " + printMock(servers) + " " + printStats(servers)); - List plans = loadBalancer.balanceCluster(servers); - List balancedCluster = reconcile(servers, plans); - LOG.info("Mock Balance : " + printMock(balancedCluster)); - assertClusterAsBalanced(balancedCluster); - for(Map.Entry> entry : servers.entrySet()) { - returnRegions(entry.getValue()); - returnServer(entry.getKey()); + @Test + public void testBalanceClusterWithFirstRegionsRootMeta() throws Exception { + int[] cluster = new int[] { 293, 293, 291 }; + LoadBalancer loadBalancerStub = new LoadBalancer(HBaseConfiguration + .create()) { + // do not randomize in order to create a scenario where ROOT and META is + // coming as the first region in the top two RS + List randomize(final List regions) { + return regions; } + }; + balanceCluster(cluster, true, loadBalancerStub); + } + + private void balanceCluster(int[] mockCluster, + boolean needRootMetaFirstRegions, LoadBalancer loadBalancerStub) { + Map> servers = mockClusterServers( + mockCluster, needRootMetaFirstRegions); + LOG.info("Mock Cluster : " + printMock(servers) + " " + + printStats(servers)); + List plans = null; + if (loadBalancerStub != null) { + plans = loadBalancerStub.balanceCluster(servers); + } else { + plans = loadBalancer.balanceCluster(servers); } - + List balancedCluster = reconcile(servers, plans); + LOG.info("Mock Balance : " + printMock(balancedCluster)); + assertClusterAsBalanced(balancedCluster); + for (Map.Entry> entry : servers.entrySet()) { + returnRegions(entry.getValue()); + returnServer(entry.getKey()); + } } + + @Test + public void testPlanContainsDuplicateRegions() throws Exception { + int[] mocCluster = new int[] { 6, 0, 4 }; + Map> servers = mockClusterServers( + mocCluster, false); + LOG.info("Mock Cluster : " + printMock(servers) + " " + + printStats(servers)); + List plans = new LoadBalancer(HBaseConfiguration.create()) { + //Try to fake the randomization. + List randomize(final List regions) { + ArrayList randList = new ArrayList(); + if (regions.size() == 6) { + randList.add(regions.get(2)); + randList.add(regions.get(3)); + randList.add(regions.get(4)); + randList.add(regions.get(0)); + randList.add(regions.get(5)); + randList.add(regions.get(1)); + } + return randList; + } + }.balanceCluster(servers); + boolean duplicates = false; + Set movingRegions = new HashSet(); + for (RegionPlan regionPlan : plans) { + if (!movingRegions.add(regionPlan.getRegionInfo())) + duplicates = true; + } + assertFalse("Plans contains duplicate regions", duplicates); + } /** * Invariant is that all servers have between floor(avg) and ceiling(avg) @@ -242,7 +304,7 @@ public void testImmediateAssignment() throws Exception { for(int [] mock : regionsAndServersMocks) { LOG.debug("testImmediateAssignment with " + mock[0] + " regions and " + mock[1] + " servers"); - List regions = randomRegions(mock[0]); + List regions = randomRegions(mock[0], false); List servers = randomServers(mock[1], 0); Map assignments = LoadBalancer.immediateAssignment(regions, servers); @@ -277,7 +339,7 @@ public void testBulkAssignment() throws Exception { for(int [] mock : regionsAndServersMocks) { LOG.debug("testBulkAssignment with " + mock[0] + " regions and " + mock[1] + " servers"); - List regions = randomRegions(mock[0]); + List regions = randomRegions(mock[0], false); List servers = randomServers(mock[1], 0); Map> assignments = LoadBalancer.roundRobinAssignment(regions, servers); @@ -303,7 +365,7 @@ public void testRetainAssignment() throws Exception { // Test simple case where all same servers are there List servers = randomServers(10, 10); - List regions = randomRegions(100); + List regions = randomRegions(100, false); Map existing = new TreeMap(); for (int i=0;i> mockClusterServers( - int [] mockCluster) { + int [] mockCluster, boolean needRootMetaFirstRegions) { int numServers = mockCluster.length; Map> servers = new TreeMap>(); for(int i=0;i regions = randomRegions(numRegions); + List regions = randomRegions(numRegions, + needRootMetaFirstRegions); servers.put(server, regions); } return servers; } private Queue regionQueue = new LinkedList(); - - private List randomRegions(int numRegions) { + + private List randomRegions(int numRegions, + boolean needRootMetaFirstRegions) { List regions = new ArrayList(numRegions); byte [] start = new byte[16]; byte [] end = new byte[16]; rand.nextBytes(start); rand.nextBytes(end); + if (needRootMetaFirstRegions && metaRootRegions.size() > 0) { + regions.add(metaRootRegions.get(0)); + metaRootRegions.remove(0); + numRegions = numRegions - 1; + } for(int i=0;i servers) { serverQueue.addAll(servers); } + }