Index: hbase-server/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java (revision 1387170) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java (working copy) @@ -19,6 +19,7 @@ package org.apache.hadoop.hbase.master.handler; import java.io.IOException; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.ExecutorService; @@ -27,6 +28,7 @@ import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.hbase.HRegionInfo; import org.apache.hadoop.hbase.Server; +import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableNotDisabledException; import org.apache.hadoop.hbase.TableNotFoundException; import org.apache.hadoop.hbase.catalog.CatalogTracker; @@ -36,7 +38,10 @@ import org.apache.hadoop.hbase.master.BulkAssigner; import org.apache.hadoop.hbase.master.HMaster; import org.apache.hadoop.hbase.master.MasterCoprocessorHost; +import org.apache.hadoop.hbase.master.RegionPlan; +import org.apache.hadoop.hbase.master.RegionStates; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.Pair; import org.apache.zookeeper.KeeperException; import org.cloudera.htrace.Trace; @@ -50,7 +55,7 @@ private final String tableNameStr; private final AssignmentManager assignmentManager; private final CatalogTracker ct; - + private boolean masterRestart = false; public EnableTableHandler(Server server, byte [] tableName, CatalogTracker catalogTracker, AssignmentManager assignmentManager, boolean skipTableStateCheck) @@ -60,11 +65,12 @@ this.tableNameStr = Bytes.toString(tableName); this.ct = catalogTracker; this.assignmentManager = assignmentManager; + this.masterRestart = skipTableStateCheck; + // Check if table exists if (!MetaReader.tableExists(catalogTracker, this.tableNameStr)) { throw new TableNotFoundException(Bytes.toString(tableName)); } - // There could be multiple client requests trying to disable or enable // the table at the same time. Ensure only the first request is honored // After that, no other requests can be accepted until the table reaches @@ -111,10 +117,12 @@ LOG.error("Error trying to enable the table " + this.tableNameStr, e); } catch (KeeperException e) { LOG.error("Error trying to enable the table " + this.tableNameStr, e); + } catch (InterruptedException e) { + LOG.error("Error trying to enable the table " + this.tableNameStr, e); } } - private void handleEnableTable() throws IOException, KeeperException { + private void handleEnableTable() throws IOException, KeeperException, InterruptedException { // I could check table is disabling and if so, not enable but require // that user first finish disabling but that might be obnoxious. @@ -123,18 +131,18 @@ boolean done = false; // Get the regions of this table. We're done when all listed // tables are onlined. - List regionsInMeta; - regionsInMeta = MetaReader.getTableRegions(this.ct, tableName, true); - int countOfRegionsInTable = regionsInMeta.size(); - List regions = regionsToAssign(regionsInMeta); + List> tableRegionsAndLocations = MetaReader + .getTableRegionsAndLocations(this.ct, tableName, true); + int countOfRegionsInTable = tableRegionsAndLocations.size(); + List regions = regionsToAssignWithServerName(tableRegionsAndLocations); int regionsCount = regions.size(); if (regionsCount == 0) { done = true; } LOG.info("Table '" + this.tableNameStr + "' has " + countOfRegionsInTable + " regions, of which " + regionsCount + " are offline."); - BulkEnabler bd = new BulkEnabler(this.server, regions, - countOfRegionsInTable); + BulkEnabler bd = new BulkEnabler(this.server, regions, countOfRegionsInTable, + this.masterRestart); try { if (bd.bulkAssign()) { done = true; @@ -158,21 +166,32 @@ } /** - * @param regionsInMeta This datastructure is edited by this method. - * @return The regionsInMeta list minus the regions that have - * been onlined; i.e. List of regions that need onlining. + * @param regionsInMeta + * @return List of regions neither in transition nor assigned. * @throws IOException */ - private List regionsToAssign( - final List regionsInMeta) - throws IOException { - final List onlineRegions = - this.assignmentManager.getRegionStates() - .getRegionsOfTable(tableName); - regionsInMeta.removeAll(onlineRegions); - return regionsInMeta; + private List regionsToAssignWithServerName( + final List> regionsInMeta) throws IOException { + List regions = new ArrayList(); + RegionStates regionStates = this.assignmentManager.getRegionStates(); + for (Pair regionLocation : regionsInMeta) { + HRegionInfo hri = regionLocation.getFirst(); + ServerName sn = regionLocation.getSecond(); + if (!regionStates.isRegionInTransition(hri) && !regionStates.isRegionAssigned(hri)) { + if (this.masterRestart && sn != null && this.assignmentManager.isServerOnline(sn)) { + this.assignmentManager.addPlan(hri.getEncodedName(), new RegionPlan(hri, null, sn)); + } + regions.add(hri); + } else { + if (LOG.isDebugEnabled()) { + LOG.debug("Skipping assign for the region " + hri + " during enable table " + + hri.getTableNameAsString() + " because its already in tranition or assigned."); + } + } + } + return regions; } - + /** * Run bulk enable. */ @@ -180,20 +199,24 @@ private final List regions; // Count of regions in table at time this assign was launched. private final int countOfRegionsInTable; + private final boolean masterRestart; BulkEnabler(final Server server, final List regions, - final int countOfRegionsInTable) { + final int countOfRegionsInTable, boolean masterRestart) { super(server); this.regions = regions; this.countOfRegionsInTable = countOfRegionsInTable; + this.masterRestart = masterRestart; } + @Override protected void populatePool(ExecutorService pool) throws IOException { boolean roundRobinAssignment = this.server.getConfiguration().getBoolean( "hbase.master.enabletable.roundrobin", false); - - if (!roundRobinAssignment) { + // In case of masterRestart always go with single assign. Going thro + // roundRobinAssignment will use bulkassign which may lead to double assignment. + if (masterRestart || !roundRobinAssignment) { for (HRegionInfo region : regions) { if (assignmentManager.getRegionStates() .isRegionInTransition(region)) { @@ -202,7 +225,12 @@ final HRegionInfo hri = region; pool.execute(Trace.wrap(new Runnable() { public void run() { - assignmentManager.assign(hri, true); + if (masterRestart) { + // Already plan is populated. + assignmentManager.assign(hri, true, false, false); + } else { + assignmentManager.assign(hri, true); + } } })); } Index: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java =================================================================== --- hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java (revision 1387170) +++ hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java (working copy) @@ -54,6 +54,7 @@ import org.apache.hadoop.hbase.master.RegionState.State; import org.apache.hadoop.hbase.master.balancer.DefaultLoadBalancer; import org.apache.hadoop.hbase.master.balancer.LoadBalancerFactory; +import org.apache.hadoop.hbase.master.handler.EnableTableHandler; import org.apache.hadoop.hbase.master.handler.ServerShutdownHandler; import org.apache.hadoop.hbase.protobuf.ProtobufUtil; import org.apache.hadoop.hbase.protobuf.generated.ClientProtos.GetRequest; @@ -97,6 +98,8 @@ private static final HRegionInfo REGIONINFO = new HRegionInfo(Bytes.toBytes("t"), HConstants.EMPTY_START_ROW, HConstants.EMPTY_START_ROW); + private static int assignmentCount; + private static boolean enabling = false; // Mocked objects or; get redone for each test. private Server server; @@ -850,6 +853,41 @@ } /** + * Test verifies whether all the enabling table regions assigned only once during master startup. + * + * @throws KeeperException + * @throws IOException + * @throws Exception + */ + @Test + public void testMasterRestartWhenTableInEnabling() throws KeeperException, IOException, Exception { + enabling = true; + List destServers = new ArrayList(1); + destServers.add(SERVERNAME_A); + Mockito.when(this.serverManager.createDestinationServersList()).thenReturn(destServers); + Mockito.when(this.serverManager.isServerOnline(SERVERNAME_A)).thenReturn(true); + HTU.getConfiguration().setInt(HConstants.MASTER_PORT, 0); + Server server = new HMaster(HTU.getConfiguration()); + AssignmentManagerWithExtrasForTesting am = setUpMockedAssignmentManager(server, + this.serverManager); + try { + // set table in enabling state. + am.getZKTable().setEnablingTable(REGIONINFO.getTableNameAsString()); + new EnableTableHandler(server, REGIONINFO.getTableName(), am.getCatalogTracker(), am, true) + .process(); + assertEquals("Number of assignments should be 1.", 1, assignmentCount); + assertTrue("Table should be enabled.", + am.getZKTable().isEnabledTable(REGIONINFO.getTableNameAsString())); + } finally { + enabling = false; + assignmentCount = 0; + am.getZKTable().setEnabledTable(REGIONINFO.getTableNameAsString()); + am.shutdown(); + ZKAssign.deleteAllNodes(this.watcher); + } + } + + /** * Creates a new ephemeral node in the SPLITTING state for the specified region. * Create it ephemeral in case regionserver dies mid-split. * @@ -921,11 +959,18 @@ // Get a meta row result that has region up on SERVERNAME_A for REGIONINFO Result r = MetaMockingUtil.getMetaTableRowResult(REGIONINFO, SERVERNAME_A); ScanResponse.Builder builder = ScanResponse.newBuilder(); + builder.addResult(ProtobufUtil.toResult(r)); builder.setMoreResults(true); - builder.addResult(ProtobufUtil.toResult(r)); - Mockito.when(ri.scan( - (RpcController)Mockito.any(), (ScanRequest)Mockito.any())). - thenReturn(builder.build()); + + if (enabling) { + Mockito.when(ri.scan((RpcController) Mockito.any(), (ScanRequest) Mockito.any())) + .thenReturn(builder.build()).thenReturn(builder.build()).thenReturn(builder.build()) + .thenReturn(builder.build()).thenReturn(builder.build()) + .thenReturn(ScanResponse.newBuilder().setMoreResults(false).build()); + } else { + Mockito.when(ri.scan((RpcController) Mockito.any(), (ScanRequest) Mockito.any())).thenReturn( + builder.build()); + } // If a get, return the above result too for REGIONINFO GetResponse.Builder getBuilder = GetResponse.newBuilder(); getBuilder.setResult(ProtobufUtil.toResult(r)); @@ -987,10 +1032,15 @@ @Override public void assign(HRegionInfo region, boolean setOfflineInZK, boolean forceNewPlan, boolean hijack) { - super.assign(region, setOfflineInZK, forceNewPlan, hijack); - this.gate.set(true); + if (enabling) { + assignmentCount++; + this.regionOnline(region, SERVERNAME_A); + } else { + super.assign(region, setOfflineInZK, forceNewPlan, hijack); + this.gate.set(true); + } } - + @Override public void assign(java.util.List regions, java.util.List servers) {