From e196b203ac981c73638c2f116e29222749465ff4 Mon Sep 17 00:00:00 2001 From: Enis Soztutar Date: Mon, 3 Mar 2014 20:07:04 +0000 Subject: [PATCH] HBASE-10632 Region lost in limbo after ArrayIndexOutOfBoundsException during assignment git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@1573723 13f79535-47bb-0310-9956-ffa450edef68 --- .../master/balancer/BaseLoadBalancer.java | 19 +++- .../balancer/StochasticLoadBalancer.java | 7 +- .../master/balancer/TestBaseLoadBalancer.java | 100 ++++++++++++++++++ 3 files changed, 121 insertions(+), 5 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 98d8eebf48b..f02ef1dd3c8 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 @@ -19,17 +19,16 @@ package org.apache.hadoop.hbase.master.balancer; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Comparator; import java.util.Deque; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.NavigableMap; import java.util.Random; import java.util.Set; import java.util.TreeMap; -import java.util.NavigableMap; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -140,7 +139,13 @@ public abstract class BaseLoadBalancer implements LoadBalancer { servers[serverIndex] = entry.getKey(); } - regionsPerServer[serverIndex] = new int[entry.getValue().size()]; + if (regionsPerServer[serverIndex] != null) { + // there is another server with the same hostAndPort in ClusterState. + // allocate the array for the total size + regionsPerServer[serverIndex] = new int[entry.getValue().size() + regionsPerServer[serverIndex].length]; + } else { + regionsPerServer[serverIndex] = new int[entry.getValue().size()]; + } serverIndicesSortedByRegionCount[serverIndex] = serverIndex; } @@ -181,7 +186,7 @@ public abstract class BaseLoadBalancer implements LoadBalancer { for (int i=0; i < loc.size(); i++) { regionLocations[regionIndex][i] = loc.get(i) == null ? -1 : - (serversToIndex.get(loc.get(i)) == null ? -1 : serversToIndex.get(loc.get(i))); + (serversToIndex.get(loc.get(i).getHostAndPort()) == null ? -1 : serversToIndex.get(loc.get(i).getHostAndPort())); } } @@ -369,10 +374,12 @@ public abstract class BaseLoadBalancer implements LoadBalancer { return this.config; } + @Override public void setClusterStatus(ClusterStatus st) { // Not used except for the StocasticBalancer } + @Override public void setMasterServices(MasterServices masterServices) { this.services = masterServices; } @@ -422,6 +429,7 @@ public abstract class BaseLoadBalancer implements LoadBalancer { * @return map of server to the regions it should take, or null if no * assignment is possible (ie. no regions or no servers) */ + @Override public Map> roundRobinAssignment(List regions, List servers) { metricsBalancer.incrMiscInvocations(); @@ -467,6 +475,7 @@ public abstract class BaseLoadBalancer implements LoadBalancer { * @param servers * @return map of regions to the server it should be assigned to */ + @Override public Map immediateAssignment(List regions, List servers) { metricsBalancer.incrMiscInvocations(); @@ -481,6 +490,7 @@ public abstract class BaseLoadBalancer implements LoadBalancer { /** * Used to assign a single region to a random server. */ + @Override public ServerName randomAssignment(HRegionInfo regionInfo, List servers) { metricsBalancer.incrMiscInvocations(); @@ -508,6 +518,7 @@ public abstract class BaseLoadBalancer implements LoadBalancer { * @param servers available servers * @return map of servers and regions to be assigned to them */ + @Override public Map> retainAssignment(Map regions, List servers) { // Update metrics 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 964d4f9140d..be849f7bdae 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 @@ -549,7 +549,9 @@ public class StochasticLoadBalancer extends BaseLoadBalancer { idx++; } - return idx; + return idx < regionLocations.length + ? regionLocations[idx] + : pickOtherRandomServer(cluster, thisServer); } void setServices(MasterServices services) { @@ -824,6 +826,7 @@ public class StochasticLoadBalancer extends BaseLoadBalancer { } + @Override double cost(Cluster cluster) { if (clusterStatus == null || loads == null) { return 0; @@ -891,6 +894,7 @@ public class StochasticLoadBalancer extends BaseLoadBalancer { } + @Override protected double getCostFromRl(RegionLoad rl) { return rl.getReadRequestsCount(); } @@ -911,6 +915,7 @@ public class StochasticLoadBalancer extends BaseLoadBalancer { this.setMultiplier(conf.getFloat(WRITE_REQUEST_COST_KEY, DEFAULT_WRITE_REQUEST_COST)); } + @Override protected double getCostFromRl(RegionLoad rl) { return rl.getWriteRequestsCount(); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestBaseLoadBalancer.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestBaseLoadBalancer.java index d0cf4fa76cd..17f00f65e33 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestBaseLoadBalancer.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestBaseLoadBalancer.java @@ -19,14 +19,18 @@ package org.apache.hadoop.hbase.master.balancer; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; +import org.apache.commons.lang.ArrayUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; @@ -36,10 +40,13 @@ import org.apache.hadoop.hbase.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.hbase.master.balancer.BaseLoadBalancer.Cluster; import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; +import com.google.common.collect.Lists; + @Category(MediumTests.class) public class TestBaseLoadBalancer extends BalancerTestBase { @@ -228,4 +235,97 @@ public class TestBaseLoadBalancer extends BalancerTestBase { } } + @Test + public void testClusterServersWithSameHostPort() { + // tests whether the BaseLoadBalancer.Cluster can be constructed with servers + // sharing same host and port + List servers = getListOfServerNames(randomServers(10, 10)); + List regions = randomRegions(101); + Map> clusterState = new HashMap>(); + + assignRegions(regions, servers, clusterState); + + // construct another list of servers, but sharing same hosts and ports + List oldServers = new ArrayList(servers.size()); + for (ServerName sn : servers) { + // The old server would have had same host and port, but different start code! + oldServers.add(ServerName.valueOf(sn.getHostname(), sn.getPort(), sn.getStartcode() - 10)); + } + + regions = randomRegions(9); // some more regions + assignRegions(regions, oldServers, clusterState); + + // should not throw exception: + BaseLoadBalancer.Cluster cluster = new Cluster(clusterState, null, null); + assertEquals(101 + 9, cluster.numRegions); + assertEquals(10, cluster.numServers); // only 10 servers because they share the same host + port + } + + private void assignRegions(List regions, List servers, + Map> clusterState) { + for (int i = 0; i < regions.size(); i++) { + ServerName sn = servers.get(i % servers.size()); + List regionsOfServer = clusterState.get(sn); + if (regionsOfServer == null) { + regionsOfServer = new ArrayList(10); + clusterState.put(sn, regionsOfServer); + } + + regionsOfServer.add(regions.get(i)); + } + } + + @Test + public void testClusterRegionLocations() { + // tests whether region locations are handled correctly in Cluster + List servers = getListOfServerNames(randomServers(10, 10)); + List regions = randomRegions(101); + Map> clusterState = new HashMap>(); + + assignRegions(regions, servers, clusterState); + + // mock block locality for some regions + RegionLocationFinder locationFinder = mock(RegionLocationFinder.class); + // block locality: region:0 => {server:0} + // region:1 => {server:0, server:1} + // region:42 => {server:4, server:9, server:5} + when(locationFinder.getTopBlockLocations(regions.get(0))).thenReturn( + Lists.newArrayList(servers.get(0))); + when(locationFinder.getTopBlockLocations(regions.get(1))).thenReturn( + Lists.newArrayList(servers.get(0), servers.get(1))); + when(locationFinder.getTopBlockLocations(regions.get(42))).thenReturn( + Lists.newArrayList(servers.get(4), servers.get(9), servers.get(5))); + + BaseLoadBalancer.Cluster cluster = new Cluster(clusterState, null, locationFinder); + + int r0 = ArrayUtils.indexOf(cluster.regions, regions.get(0)); // this is ok, it is just a test + int r1 = ArrayUtils.indexOf(cluster.regions, regions.get(1)); + int r10 = ArrayUtils.indexOf(cluster.regions, regions.get(10)); + int r42 = ArrayUtils.indexOf(cluster.regions, regions.get(42)); + + int s0 = cluster.serversToIndex.get(servers.get(0).getHostAndPort()); + int s1 = cluster.serversToIndex.get(servers.get(1).getHostAndPort()); + int s4 = cluster.serversToIndex.get(servers.get(4).getHostAndPort()); + int s5 = cluster.serversToIndex.get(servers.get(5).getHostAndPort()); + int s9 = cluster.serversToIndex.get(servers.get(9).getHostAndPort()); + + // region 0 locations + assertEquals(1, cluster.regionLocations[r0].length); + assertEquals(s0, cluster.regionLocations[r0][0]); + + // region 1 locations + assertEquals(2, cluster.regionLocations[r1].length); + assertEquals(s0, cluster.regionLocations[r1][0]); + assertEquals(s1, cluster.regionLocations[r1][1]); + + // region 10 locations + assertEquals(0, cluster.regionLocations[r10].length); + + // region 42 locations + assertEquals(3, cluster.regionLocations[r42].length); + assertEquals(s4, cluster.regionLocations[r42][0]); + assertEquals(s9, cluster.regionLocations[r42][1]); + assertEquals(s5, cluster.regionLocations[r42][2]); + } + }