From 0f9ee513e247edcf60c26b990c559cad91076fee Mon Sep 17 00:00:00 2001 From: Michael Stack Date: Mon, 12 Mar 2018 16:08:41 -0700 Subject: [PATCH] HBASE-20178 [AMv2] Throw exception if hostile environment Fail-fast by throwing exception out of Procedure constructor if move but table is disabled, etc. M hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMove.java Test stolen from HBASE-20131 M hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableState.java Add convenience isEnabled/isDisabled M hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java Promote assert state to throw exception. M hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java Add isClusterUp M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java Move constructor now throws exception M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TruncateTableProcedure.java Do environment check at construction and fail-fast if hostile. M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineTableProcedure.java Add checkTableOnline utility method. --- .../org/apache/hadoop/hbase/client/TableState.java | 27 +++-- .../hadoop/hbase/zookeeper/ReadOnlyZKClient.java | 2 +- .../org/apache/hadoop/hbase/ipc/CallRunner.java | 3 +- .../org/apache/hadoop/hbase/master/HMaster.java | 6 +- .../hadoop/hbase/master/MasterRpcServices.java | 5 +- .../apache/hadoop/hbase/master/MasterServices.java | 5 + .../hadoop/hbase/master/TableNamespaceManager.java | 4 +- .../hadoop/hbase/master/TableStateManager.java | 14 +-- .../hbase/master/assignment/AssignProcedure.java | 3 +- .../hbase/master/assignment/AssignmentManager.java | 14 ++- .../assignment/MergeTableRegionsProcedure.java | 7 +- .../master/assignment/MoveRegionProcedure.java | 12 +- .../assignment/SplitTableRegionProcedure.java | 3 + .../AbstractStateMachineTableProcedure.java | 32 ++++++ .../master/procedure/DeleteNamespaceProcedure.java | 6 +- .../master/procedure/DisableTableProcedure.java | 17 ++- .../master/procedure/EnableTableProcedure.java | 9 +- .../master/procedure/ModifyTableProcedure.java | 8 +- .../master/procedure/RestoreSnapshotProcedure.java | 8 +- .../master/procedure/TruncateTableProcedure.java | 8 +- .../hadoop/hbase/regionserver/HRegionServer.java | 3 +- .../hbase/regionserver/RegionServerServices.java | 5 + .../resources/hbase-webapps/master/rsgroup.jsp | 9 +- .../hadoop/hbase/MockRegionServerServices.java | 5 + .../hbase/master/MockNoopMasterServices.java | 5 + .../hadoop/hbase/master/MockRegionServer.java | 5 + .../procedure/MasterProcedureTestingUtility.java | 4 +- .../hadoop/hbase/regionserver/TestRegionMove.java | 128 +++++++++++++++++++++ 28 files changed, 293 insertions(+), 64 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMove.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableState.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableState.java index d2334a4266..6eea40f3e1 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableState.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableState.java @@ -97,6 +97,22 @@ public class TableState { private final State state; /** + * @return True if table is enabled: i.e. {@link State#ENABLED} or {@link State#ENABLING}. + */ + public boolean isEnabled() { + return isInStates(this.state, State.ENABLED, State.ENABLING); + } + + /** + * @return True if table is disabled: i.e. NOT {@link State#ENABLED} and NOT + * {@link State#ENABLING}. + */ + public boolean isDisabled() { + return !isEnabled(); + } + + + /** * Create instance of TableState. * @param tableName name of the table * @param state table state @@ -177,14 +193,14 @@ public class TableState { /** * Static version of state checker - * @param state desired * @param target equals to any of * @return true if satisfies */ - public static boolean isInStates(State state, State... target) { + public boolean isInStates(State... target) { for (State tableState : target) { - if (state.equals(tableState)) + if (this.state.equals(tableState)) { return true; + } } return false; } @@ -212,9 +228,6 @@ public class TableState { @Override public String toString() { - return "TableState{" + - ", tableName=" + tableName + - ", state=" + state + - '}'; + return "tableName=" + tableName + ", state=" + state; } } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ReadOnlyZKClient.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ReadOnlyZKClient.java index e41f04acbf..d2f4763332 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ReadOnlyZKClient.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ReadOnlyZKClient.java @@ -313,7 +313,7 @@ public final class ReadOnlyZKClient implements Closeable { } if (task == null) { if (pendingRequests == 0) { - LOG.debug("{} to {} inactive for {}ms; closing (Will reconnect when new requests)", + LOG.trace("{} to {} inactive for {}ms; closing (Will reconnect when new requests)", getId(), connectString, keepAliveTimeMs); closeZk(); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/CallRunner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/CallRunner.java index e18518ebaa..e4763a5c5d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/CallRunner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/CallRunner.java @@ -138,7 +138,8 @@ public class CallRunner { RpcServer.LOG.trace(call.toShortString(), e); } } else { - RpcServer.LOG.debug(call.toShortString(), e); + // Don't dump full exception.. just String version + RpcServer.LOG.debug(call.toShortString() + ", exception=" + e); } errorThrowable = e; error = StringUtils.stringifyException(e); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index cd967ed4d8..36f5a291cf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -1681,7 +1681,6 @@ public class HMaster extends HRegionServer implements MasterServices { // Now we can do the move RegionPlan rp = new RegionPlan(hri, regionState.getServerName(), dest); assert rp.getDestination() != null: rp.toString() + " " + dest; - assert rp.getSource() != null: rp.toString(); try { checkInitialized(); @@ -2398,8 +2397,9 @@ public class HMaster extends HRegionServer implements MasterServices { throw new IOException("Can't modify catalog tables"); } checkTableExists(tableName); - if (!getTableStateManager().isTableState(tableName, TableState.State.DISABLED)) { - throw new TableNotDisabledException(tableName); + TableState ts = getTableStateManager().getTableState(tableName); + if (!ts.getState().equals(TableState.State.DISABLED)) { + throw new TableNotDisabledException(tableName + " is not disabled, state=" + ts.getState()); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java index 8f92041f6b..97a3c406a8 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java @@ -1038,10 +1038,9 @@ public class MasterRpcServices extends RSRpcServices try { master.checkServiceStarted(); TableName tableName = ProtobufUtil.toTableName(request.getTableName()); - TableState.State state = master.getTableStateManager() - .getTableState(tableName); + TableState ts = master.getTableStateManager().getTableState(tableName); GetTableStateResponse.Builder builder = GetTableStateResponse.newBuilder(); - builder.setTableState(new TableState(tableName, state).convert()); + builder.setTableState(new TableState(tableName, ts.getState()).convert()); return builder.build(); } catch (IOException e) { throw new ServiceException(e); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java index 0e552d6930..52046c5149 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java @@ -501,4 +501,9 @@ public interface MasterServices extends Server { boolean recoverMeta() throws IOException; String getClientIdAuditPrefix(); + + /** + * @return True if cluster is up; false if cluster is not up (we are shutting down). + */ + boolean isClusterUp(); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableNamespaceManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableNamespaceManager.java index f334d32ae4..b2d6d97a59 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableNamespaceManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableNamespaceManager.java @@ -315,12 +315,12 @@ public class TableNamespaceManager implements Stoppable { return false; } - private TableState.State getTableState() throws IOException { + private TableState getTableState() throws IOException { return masterServices.getTableStateManager().getTableState(TableName.NAMESPACE_TABLE_NAME); } private boolean isTableEnabled() throws IOException { - return getTableState().equals(TableState.State.ENABLED); + return getTableState().getState().equals(TableState.State.ENABLED); } private boolean isTableAssigned() { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java index affb684f74..b5c924acf6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java @@ -141,8 +141,8 @@ public class TableStateManager { public boolean isTableState(TableName tableName, TableState.State... states) { try { - TableState.State tableState = getTableState(tableName); - return TableState.isInStates(tableState, states); + TableState tableState = getTableState(tableName); + return tableState.isInStates(states); } catch (IOException e) { LOG.error("Unable to get table " + tableName + " state", e); return false; @@ -188,12 +188,12 @@ public class TableStateManager { } @NonNull - public TableState.State getTableState(TableName tableName) throws IOException { + public TableState getTableState(TableName tableName) throws IOException { TableState currentState = readMetaState(tableName); if (currentState == null) { throw new TableStateNotFoundException(tableName); } - return currentState.getState(); + return currentState; } protected void updateMetaState(TableName tableName, TableState.State newState) @@ -283,13 +283,13 @@ public class TableStateManager { entry.getKey()); continue; } - TableState.State state = null; + TableState ts = null; try { - state = getTableState(entry.getKey()); + ts = getTableState(entry.getKey()); } catch (TableStateNotFoundException e) { // This can happen; table exists but no TableState. } - if (state == null) { + if (ts == null) { TableState.State zkstate = entry.getValue(); // Only migrate if it is an enable or disabled table. If in-between -- ENABLING or // DISABLING then we have a problem; we are starting up an hbase-2 on a cluster with diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java index fd3155398f..7f58bd5049 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java @@ -26,7 +26,6 @@ import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.client.RetriesExhaustedException; -import org.apache.hadoop.hbase.client.TableState; import org.apache.hadoop.hbase.exceptions.UnexpectedStateException; import org.apache.hadoop.hbase.master.RegionState.State; import org.apache.hadoop.hbase.master.TableStateManager; @@ -161,7 +160,7 @@ public class AssignProcedure extends RegionTransitionProcedure { // Don't assign if table is in disabling or disabled state. TableStateManager tsm = env.getMasterServices().getTableStateManager(); TableName tn = regionNode.getRegionInfo().getTable(); - if (tsm.isTableState(tn, TableState.State.DISABLING, TableState.State.DISABLED)) { + if (tsm.getTableState(tn).isDisabled()) { LOG.info("Table " + tn + " state=" + tsm.getTableState(tn) + ", skipping " + this); return false; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java index 33a8d2174b..754731b02b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java @@ -556,7 +556,7 @@ public class AssignmentManager implements ServerListener { ProcedureSyncWait.submitAndWaitProcedure(master.getMasterProcedureExecutor(), proc); } - public Future moveAsync(final RegionPlan regionPlan) { + public Future moveAsync(final RegionPlan regionPlan) throws HBaseIOException { MoveRegionProcedure proc = createMoveRegionProcedure(regionPlan); return ProcedureSyncWait.submitProcedure(master.getMasterProcedureExecutor(), proc); } @@ -678,7 +678,7 @@ public class AssignmentManager implements ServerListener { return procedures.toArray(ASSIGN_PROCEDURE_ARRAY_TYPE); } - // Needed for the following method so it can type the created Array we return + // Needed for the following method so it can type the created Array we retur n private static final UnassignProcedure [] UNASSIGN_PROCEDURE_ARRAY_TYPE = new UnassignProcedure[0]; @@ -695,7 +695,8 @@ public class AssignmentManager implements ServerListener { return procs.toArray(UNASSIGN_PROCEDURE_ARRAY_TYPE); } - public MoveRegionProcedure[] createReopenProcedures(final Collection regionInfo) { + public MoveRegionProcedure[] createReopenProcedures(final Collection regionInfo) + throws IOException { final MoveRegionProcedure[] procs = new MoveRegionProcedure[regionInfo.size()]; int index = 0; for (RegionInfo hri: regionInfo) { @@ -744,7 +745,8 @@ public class AssignmentManager implements ServerListener { return proc; } - public MoveRegionProcedure createMoveRegionProcedure(final RegionPlan plan) { + public MoveRegionProcedure createMoveRegionProcedure(final RegionPlan plan) + throws HBaseIOException { if (plan.getRegionInfo().getTable().isSystemTable()) { List exclude = getExcludedServersForSystemTable(); if (plan.getDestination() != null && exclude.contains(plan.getDestination())) { @@ -861,8 +863,8 @@ public class AssignmentManager implements ServerListener { final ServerStateNode serverNode = regionStates.getOrCreateServer(serverName); if (!reportTransition(regionNode, serverNode, state, seqId)) { - LOG.warn(String.format( - "No procedure for %s. server=%s to transition to %s", regionNode, serverName, state)); + // Don't log if shutting down cluster; during shutdown. + LOG.warn("No matchin procedure found for {} to transition to {}", regionNode, state); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java index 052ba7fbd3..ec59d15bfd 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java @@ -73,6 +73,8 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.M * The procedure to Merge a region in a table. * This procedure takes an exclusive table lock since it is working over multiple regions. * It holds the lock for the life of the procedure. + *

Throws exception on construction if determines context hostile to merge (cluster going + * down or master is shutting down or table is disabled).

*/ @InterfaceAudience.Private public class MergeTableRegionsProcedure @@ -96,14 +98,15 @@ public class MergeTableRegionsProcedure public MergeTableRegionsProcedure(final MasterProcedureEnv env, final RegionInfo regionToMergeA, final RegionInfo regionToMergeB, - final boolean forcible) throws MergeRegionException { + final boolean forcible) throws IOException { this(env, new RegionInfo[] {regionToMergeA, regionToMergeB}, forcible); } public MergeTableRegionsProcedure(final MasterProcedureEnv env, final RegionInfo[] regionsToMerge, final boolean forcible) - throws MergeRegionException { + throws IOException { super(env); + checkTableOnline(env); // Check daughter regions and make sure that we have valid daughter regions // before doing the real work. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java index a29bfee2cb..733b9eb265 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java @@ -21,6 +21,7 @@ package org.apache.hadoop.hbase.master.assignment; import java.io.IOException; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.RegionInfo; @@ -40,6 +41,9 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.M * It first runs an unassign subprocedure followed * by an assign subprocedure. It takes a lock on the region being moved. * It holds the lock for the life of the procedure. + * + *

Throws exception on construction if determines context hostile to move (cluster going + * down or master is shutting down or table is disabled).

*/ @InterfaceAudience.Private public class MoveRegionProcedure extends AbstractStateMachineRegionProcedure { @@ -51,9 +55,15 @@ public class MoveRegionProcedure extends AbstractStateMachineRegionProcedureThrows exception on construction if determines context hostile to spllt (cluster going + * down or master is shutting down or table is disabled).

*/ @InterfaceAudience.Private public class SplitTableRegionProcedure @@ -104,6 +106,7 @@ public class SplitTableRegionProcedure public SplitTableRegionProcedure(final MasterProcedureEnv env, final RegionInfo regionToSplit, final byte[] splitRow) throws IOException { super(env, regionToSplit); + checkTableOnline(env); this.bestSplitRow = splitRow; checkSplittable(env, regionToSplit, bestSplitRow); final TableName table = regionToSplit.getTable(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineTableProcedure.java index 833b659a5b..98f913daca 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/AbstractStateMachineTableProcedure.java @@ -20,11 +20,15 @@ package org.apache.hadoop.hbase.master.procedure; import java.io.IOException; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.MetaTableAccessor; import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.TableNotEnabledException; import org.apache.hadoop.hbase.TableNotFoundException; import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.TableState; import org.apache.hadoop.hbase.master.MasterFileSystem; +import org.apache.hadoop.hbase.master.MasterServices; import org.apache.hadoop.hbase.procedure2.StateMachineProcedure; import org.apache.hadoop.hbase.security.User; import org.apache.hadoop.hbase.util.FSUtils; @@ -124,4 +128,32 @@ public abstract class AbstractStateMachineTableProcedure Path tableDir = FSUtils.getTableDir(mfs.getRootDir(), getTableName()); return new Path(tableDir, ServerRegionReplicaUtil.getRegionInfoForFs(region).getEncodedName()); } + + /** + * Check table is online and modifiable and that cluster and master are up. + * Run in constructor so can pass exception to caller. + */ + protected void checkTableOnline(MasterProcedureEnv env) throws HBaseIOException { + MasterServices master = env.getMasterServices(); + if (!master.isClusterUp()) { + throw new HBaseIOException("Cluster not up!"); + } + if (master.isStopping() || master.isStopped()) { + throw new HBaseIOException("Master stopping=" + master.isStopping() + + ", stopped=" + master.isStopped()); + } + try { + TableName tn = getTableName(); + TableState ts = master.getTableStateManager().getTableState(tn); + if (ts.isEnabled()) { + throw new TableNotEnabledException(tn); + } + checkTableModifiable(env); + } catch (IOException ioe) { + if (ioe instanceof HBaseIOException) { + throw (HBaseIOException)ioe; + } + throw new HBaseIOException(ioe); + } + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteNamespaceProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteNamespaceProcedure.java index 5f7959e410..8369a19564 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteNamespaceProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteNamespaceProcedure.java @@ -71,11 +71,7 @@ public class DeleteNamespaceProcedure @Override protected Flow executeFromState(final MasterProcedureEnv env, final DeleteNamespaceState state) throws InterruptedException { - if (isTraceEnabled()) { - LOG.trace(this + " execute state=" + state); - } - LOG.info(this + " execute state=" + state); - + LOG.info(this.toString()); try { switch (state) { case DELETE_NAMESPACE_PREPARE: diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java index e748c6ce7f..6b70687fb5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DisableTableProcedure.java @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.master.procedure; import java.io.IOException; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.MetaTableAccessor; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNotEnabledException; @@ -57,7 +58,8 @@ public class DisableTableProcedure * @param skipTableStateCheck whether to check table state */ public DisableTableProcedure(final MasterProcedureEnv env, final TableName tableName, - final boolean skipTableStateCheck) { + final boolean skipTableStateCheck) + throws HBaseIOException { this(env, tableName, skipTableStateCheck, null); } @@ -68,9 +70,11 @@ public class DisableTableProcedure * @param skipTableStateCheck whether to check table state */ public DisableTableProcedure(final MasterProcedureEnv env, final TableName tableName, - final boolean skipTableStateCheck, final ProcedurePrepareLatch syncLatch) { + final boolean skipTableStateCheck, final ProcedurePrepareLatch syncLatch) + throws HBaseIOException { super(env, syncLatch); this.tableName = tableName; + checkTableOnline(env); this.skipTableStateCheck = skipTableStateCheck; } @@ -230,11 +234,12 @@ public class DisableTableProcedure // was implemented. With table lock, there is no need to set the state here (it will // set the state later on). A quick state check should be enough for us to move forward. TableStateManager tsm = env.getMasterServices().getTableStateManager(); - TableState.State state = tsm.getTableState(tableName); - if (!state.equals(TableState.State.ENABLED)){ - LOG.info("Table " + tableName + " isn't enabled;is "+state.name()+"; skipping disable"); + TableState ts = tsm.getTableState(tableName); + if (!ts.getState().equals(TableState.State.ENABLED)){ + LOG.info("Table " + tableName + " isn't enabled; is " + ts.getState().name() + + "; skipping disable"); setFailure("master-disable-table", new TableNotEnabledException( - tableName+" state is "+state.name())); + tableName+" state is " + ts.getState().name())); canTableBeDisabled = false; } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java index c501e5396a..f2bb905526 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java @@ -333,11 +333,12 @@ public class EnableTableProcedure // was implemented. With table lock, there is no need to set the state here (it will // set the state later on). A quick state check should be enough for us to move forward. TableStateManager tsm = env.getMasterServices().getTableStateManager(); - TableState.State state = tsm.getTableState(tableName); - if(!state.equals(TableState.State.DISABLED)){ - LOG.info("Table " + tableName + " isn't disabled;is "+state.name()+"; skipping enable"); + TableState ts = tsm.getTableState(tableName); + if(!ts.getState().equals(TableState.State.DISABLED)){ + LOG.info("Table " + tableName + " isn't disabled; is " + ts.getState().name() + + "; skipping enable"); setFailure("master-enable-table", new TableNotDisabledException( - this.tableName+" state is "+state.name())); + this.tableName + " state is " + ts.getState().name())); canTableBeEnabled = false; } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java index f0be1e0819..bf6233c602 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.Set; import org.apache.hadoop.hbase.DoNotRetryIOException; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.MetaTableAccessor; import org.apache.hadoop.hbase.TableName; @@ -63,15 +64,18 @@ public class ModifyTableProcedure initilize(); } - public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor htd) { + public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor htd) + throws HBaseIOException { this(env, htd, null); } public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor htd, - final ProcedurePrepareLatch latch) { + final ProcedurePrepareLatch latch) + throws HBaseIOException { super(env, latch); initilize(); this.modifiedTableDescriptor = htd; + checkTableOnline(env); } private void initilize() { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java index 9aa5171786..b1216af9e3 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java @@ -28,6 +28,7 @@ import java.util.Map; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.DoNotRetryIOException; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.MetaTableAccessor; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNotFoundException; @@ -81,7 +82,8 @@ public class RestoreSnapshotProcedure } public RestoreSnapshotProcedure(final MasterProcedureEnv env, - final TableDescriptor tableDescriptor, final SnapshotDescription snapshot) { + final TableDescriptor tableDescriptor, final SnapshotDescription snapshot) + throws HBaseIOException { this(env, tableDescriptor, snapshot, false); } /** @@ -95,10 +97,12 @@ public class RestoreSnapshotProcedure final MasterProcedureEnv env, final TableDescriptor tableDescriptor, final SnapshotDescription snapshot, - final boolean restoreAcl) { + final boolean restoreAcl) + throws HBaseIOException { super(env); // This is the new schema we are going to write out as this modification. this.modifiedTableDescriptor = tableDescriptor; + checkTableOnline(env); // Snapshot information this.snapshot = snapshot; this.restoreAcl = restoreAcl; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TruncateTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TruncateTableProcedure.java index 80cc5a8047..bae93a21bf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TruncateTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TruncateTableProcedure.java @@ -23,6 +23,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNotDisabledException; import org.apache.hadoop.hbase.TableNotFoundException; @@ -56,14 +57,17 @@ public class TruncateTableProcedure } public TruncateTableProcedure(final MasterProcedureEnv env, final TableName tableName, - boolean preserveSplits) { + boolean preserveSplits) + throws HBaseIOException { this(env, tableName, preserveSplits, null); } public TruncateTableProcedure(final MasterProcedureEnv env, final TableName tableName, - boolean preserveSplits, ProcedurePrepareLatch latch) { + boolean preserveSplits, ProcedurePrepareLatch latch) + throws HBaseIOException { super(env, latch); this.tableName = tableName; + checkTableOnline(env); this.preserveSplits = preserveSplits; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index 81febc0ea9..e03757ffbd 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java @@ -910,7 +910,8 @@ public class HRegionServer extends HasThread implements /** * @return True if the cluster is up. */ - private boolean isClusterUp() { + @Override + public boolean isClusterUp() { return this.masterless || (this.clusterStatusTracker != null && this.clusterStatusTracker.isClusterUp()); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java index 60bd215933..023efd94b9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java @@ -234,4 +234,9 @@ public interface RegionServerServices extends Server, MutableOnlineRegions, Favo * See HBASE-17712 for more details. */ void unassign(byte[] regionName) throws IOException; + + /** + * @return True if cluster is up; false if cluster is not up (we are shutting down). + */ + boolean isClusterUp(); } diff --git a/hbase-server/src/main/resources/hbase-webapps/master/rsgroup.jsp b/hbase-server/src/main/resources/hbase-webapps/master/rsgroup.jsp index 43753a5b1a..ec6ca718b8 100644 --- a/hbase-server/src/main/resources/hbase-webapps/master/rsgroup.jsp +++ b/hbase-server/src/main/resources/hbase-webapps/master/rsgroup.jsp @@ -428,13 +428,12 @@ <%= tableName.getNamespaceAsString() %> <%= tableName.getQualifierAsString() %> - <% TableState.State tableState = master.getTableStateManager().getTableState(tableName); - if(TableState.isInStates(tableState, - TableState.State.DISABLED, TableState.State.DISABLING)) { + <% TableState tableState = master.getTableStateManager().getTableState(tableName); + if(tableState.isInStates(TableState.State.DISABLED, TableState.State.DISABLING)) { %> - <%= tableState.name() %> + <%= tableState.getState().name() %> <% } else { %> - <%= tableState.name() %> + <%= tableState.getState().name() %> <% } %> <% Map> tableRegions = master.getAssignmentManager().getRegionStates().getRegionByStateOfTable(tableName); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/MockRegionServerServices.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/MockRegionServerServices.java index f1e020f581..879b592f17 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/MockRegionServerServices.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/MockRegionServerServices.java @@ -334,4 +334,9 @@ public class MockRegionServerServices implements RegionServerServices { public Connection createConnection(Configuration conf) throws IOException { return null; } + + @Override + public boolean isClusterUp() { + return true; + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java index 4e66676303..cc2f43dd44 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java @@ -478,4 +478,9 @@ public class MockNoopMasterServices implements MasterServices { public ReplicationPeerManager getReplicationPeerManager() { return null; } + + @Override + public boolean isClusterUp() { + return true; + } } \ No newline at end of file diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java index cabd769698..d366b67fe5 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java @@ -678,4 +678,9 @@ ClientProtos.ClientService.BlockingInterface, RegionServerServices { public Connection createConnection(Configuration conf) throws IOException { return null; } + + @Override + public boolean isClusterUp() { + return true; + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java index 9546b19767..c8bb97de33 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java @@ -263,13 +263,13 @@ public class MasterProcedureTestingUtility { public static void validateTableIsEnabled(final HMaster master, final TableName tableName) throws IOException { TableStateManager tsm = master.getTableStateManager(); - assertTrue(tsm.getTableState(tableName).equals(TableState.State.ENABLED)); + assertTrue(tsm.getTableState(tableName).getState().equals(TableState.State.ENABLED)); } public static void validateTableIsDisabled(final HMaster master, final TableName tableName) throws IOException { TableStateManager tsm = master.getTableStateManager(); - assertTrue(tsm.getTableState(tableName).equals(TableState.State.DISABLED)); + assertTrue(tsm.getTableState(tableName).getState().equals(TableState.State.DISABLED)); } public static void validateColumnFamilyAddition(final HMaster master, final TableName tableName, diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMove.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMove.java new file mode 100644 index 0000000000..8940beaac4 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMove.java @@ -0,0 +1,128 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.regionserver; + +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.util.List; +import java.util.stream.Collectors; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.TableNotEnabledException; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.Put; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.Table; +import org.apache.hadoop.hbase.testclassification.LargeTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.ExpectedException; +import org.junit.rules.TestName; + +/** + * Test move fails when table disabled + */ +@Category({LargeTests.class}) +public class TestRegionMove { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestRegionMove.class); + + @Rule + public ExpectedException thrown = ExpectedException.none(); + + @Rule + public TestName name = new TestName(); + private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility(); + public static Configuration CONF ; + protected static final String F1 = "f1"; + + // Test names + protected TableName tableName; + protected String method; + + @BeforeClass + public static void startCluster() throws Exception { + TEST_UTIL.startMiniCluster(2); + } + + @AfterClass + public static void stopCluster() throws Exception { + TEST_UTIL.shutdownMiniCluster(); + } + + @Before + public void setup() throws IOException { + CONF = TEST_UTIL.getConfiguration(); + method = name.getMethodName(); + tableName = TableName.valueOf(method); + } + + @Test + public void testDisableAndMove() throws Exception { + Admin admin = TEST_UTIL.getAdmin(); + + // Create a table with more than one region + Table t = TEST_UTIL.createMultiRegionTable(tableName, Bytes.toBytes(F1), 10); + TEST_UTIL.waitUntilAllRegionsAssigned(tableName); + + // Write an update to each region + for (RegionInfo regionInfo : admin.getRegions(tableName)) { + byte[] startKey = regionInfo.getStartKey(); + // The startKey of the first region is "empty", which would throw an error if we try to + // Put that. + byte[] rowKey = org.apache.hbase.thirdparty.com.google.common.primitives.Bytes.concat( + startKey, Bytes.toBytes("1")); + Put p = new Put(rowKey); + p.addColumn(Bytes.toBytes(F1), Bytes.toBytes("q1"), Bytes.toBytes("value")); + t.put(p); + } + + // Get a Region which is on the first RS + HRegionServer rs1 = TEST_UTIL.getRSForFirstRegionInTable(tableName); + HRegionServer rs2 = TEST_UTIL.getOtherRegionServer(rs1); + List regionsOnRS1ForTable = admin.getRegions(rs1.getServerName()).stream() + .filter((regionInfo) -> regionInfo.getTable().equals(tableName)) + .collect(Collectors.toList()); + assertTrue( + "Expected to find at least one region for " + tableName + " on " + rs1.getServerName() + + ", but found none", !regionsOnRS1ForTable.isEmpty()); + final RegionInfo regionToMove = regionsOnRS1ForTable.get(0); + + // Disable the table + admin.disableTable(tableName); + + // We except a DNRIOE when we try to move a region which isn't open. + thrown.expect(TableNotEnabledException.class); + thrown.expectMessage(t.getName().toString()); + + // Move the region to the other RS -- should fail + admin.move(regionToMove.getEncodedNameAsBytes(), Bytes.toBytes(rs2.getServerName().toString())); + } +} -- 2.11.0 (Apple Git-81)