commit 1d6e5b4cd42f486136d5ac0f3501c3dc3f72d711 Author: Todd Lipcon Date: Wed Sep 14 23:35:38 2011 -0700 HBASE-4402. fix retain assigment diff --git src/main/java/org/apache/hadoop/hbase/master/DefaultLoadBalancer.java src/main/java/org/apache/hadoop/hbase/master/DefaultLoadBalancer.java index d608327..286ae8c 100644 --- src/main/java/org/apache/hadoop/hbase/master/DefaultLoadBalancer.java +++ src/main/java/org/apache/hadoop/hbase/master/DefaultLoadBalancer.java @@ -46,6 +46,7 @@ import org.apache.hadoop.hbase.TableExistsException; import org.apache.hadoop.hbase.regionserver.HRegion; import org.apache.hadoop.hbase.util.Bytes; +import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.MinMaxPriorityQueue; /** @@ -576,20 +577,55 @@ public class DefaultLoadBalancer implements LoadBalancer { */ public Map> retainAssignment( Map regions, List servers) { + // Group all of the old assignments by their hostname. + // We can't group directly by ServerName since the servers all have + // new start-codes. + + // Group the servers by their hostname. It's possible we have multiple + // servers on the same host on different ports. + ArrayListMultimap serversByHostname = + ArrayListMultimap.create(); + for (ServerName server : servers) { + serversByHostname.put(server.getHostname(), server); + } + + // Now come up with new assignments Map> assignments = new TreeMap>(); + for (ServerName server : servers) { assignments.put(server, new ArrayList()); } - for (Map.Entry region : regions.entrySet()) { - ServerName sn = region.getValue(); - if (sn != null && servers.contains(sn)) { - assignments.get(sn).add(region.getKey()); + + int numRandomAssignments = 0; + int numRetainedAssigments = 0; + for (Map.Entry entry : regions.entrySet()) { + HRegionInfo region = entry.getKey(); + ServerName previousServer = entry.getValue(); + List localServers = serversByHostname.get( + previousServer.getHostname()); + if (localServers.isEmpty()) { + // No servers on the new cluster match up with this hostname, + // assign randomly. + ServerName randomServer = servers.get(RANDOM.nextInt(servers.size())); + assignments.get(randomServer).add(region); + numRandomAssignments++; + } else if (localServers.size() == 1) { + // the usual case - one new server on same host + assignments.get(localServers.get(0)).add(region); + numRetainedAssigments++; } else { - int size = assignments.size(); - assignments.get(servers.get(RANDOM.nextInt(size))).add(region.getKey()); + // multiple new servers in the cluster on this same host + int size = localServers.size(); + ServerName target = localServers.get(RANDOM.nextInt(size)); + assignments.get(target).add(region); + numRetainedAssigments++; } } + + LOG.info("Reassigned " + regions.size() + " regions. " + + numRetainedAssigments + " retained the pre-restart assignment, " + + numRandomAssignments + " were re-assigned randomly."); return assignments; } diff --git src/test/java/org/apache/hadoop/hbase/master/TestDefaultLoadBalancer.java src/test/java/org/apache/hadoop/hbase/master/TestDefaultLoadBalancer.java index a4cd9a3..fb09acb 100644 --- src/test/java/org/apache/hadoop/hbase/master/TestDefaultLoadBalancer.java +++ src/test/java/org/apache/hadoop/hbase/master/TestDefaultLoadBalancer.java @@ -269,7 +269,12 @@ public class TestDefaultLoadBalancer { Map existing = new TreeMap(); for (int i = 0; i < regions.size(); i++) { - existing.put(regions.get(i), servers.get(i % servers.size()).getServerName()); + ServerName sn = servers.get(i % servers.size()).getServerName(); + // The old server would have had same host and port, but different + // start code! + ServerName snWithOldStartCode = + new ServerName(sn.getHostname(), sn.getPort(), sn.getStartcode() - 10); + existing.put(regions.get(i), snWithOldStartCode); } List listOfServerNames = getListOfServerNames(servers); Map> assignment = @@ -288,9 +293,9 @@ public class TestDefaultLoadBalancer { // Remove two of the servers that were previously there List servers3 = new ArrayList(servers); - servers3.remove(servers3.size()-1); - servers3.remove(servers3.size()-2); - listOfServerNames = getListOfServerNames(servers2); + servers3.remove(0); + servers3.remove(0); + listOfServerNames = getListOfServerNames(servers3); assignment = loadBalancer.retainAssignment(existing, listOfServerNames); assertRetainedAssignment(existing, listOfServerNames, assignment); } @@ -330,13 +335,20 @@ public class TestDefaultLoadBalancer { assertEquals(existing.size(), assignedRegions.size()); // Verify condition 2, if server had existing assignment, must have same - Set onlineAddresses = new TreeSet(); - for (ServerName s : servers) onlineAddresses.add(s); + Set onlineHostNames = new TreeSet(); + for (ServerName s : servers) { + onlineHostNames.add(s.getHostname()); + } + for (Map.Entry> a : assignment.entrySet()) { + ServerName assignedTo = a.getKey(); for (HRegionInfo r : a.getValue()) { ServerName address = existing.get(r); - if (address != null && onlineAddresses.contains(address)) { - assertTrue(a.getKey().equals(address)); + if (address != null && onlineHostNames.contains(address.getHostname())) { + // this region was prevously assigned somewhere, and that + // host is still around, then it should be re-assigned on the + // same host + assertEquals(address.getHostname(), assignedTo.getHostname()); } } } @@ -466,7 +478,7 @@ public class TestDefaultLoadBalancer { ServerName sn = this.serverQueue.poll(); return new ServerAndLoad(sn, numRegionsPerServer); } - String host = "127.0.0.1"; + String host = "server" + rand.nextInt(100000); int port = rand.nextInt(60000); long startCode = rand.nextLong(); ServerName sn = new ServerName(host, port, startCode);