HBASE-20706 Prevent MTP from trying to reopen non-OPEN regions ModifyTableProcedure is using MoveRegionProcedure in a way that was unintended from the original implementation. As such, we have to guard against certain usages of it. We know we can re-open OPEN regions, but regions in OPENING will similarly soon be OPEN (thus, we want to reopen those regions too).
Signed-off-by: Michael Stack <st...@apache.org> Signed-off-by: zhangduo <zhang...@apache.org> Project: http://git-wip-us.apache.org/repos/asf/hbase/repo Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/e989a992 Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/e989a992 Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/e989a992 Branch: refs/heads/HBASE-19064 Commit: e989a9927e4938ab64be9d82324e8a00a5768032 Parents: d1cad1a Author: Josh Elser <els...@apache.org> Authored: Wed Jun 13 16:30:33 2018 -0400 Committer: Josh Elser <els...@apache.org> Committed: Wed Jun 20 14:19:28 2018 -0700 ---------------------------------------------------------------------- .../hbase/master/assignment/RegionStates.java | 27 ++++++++++++++++++-- .../master/procedure/ModifyTableProcedure.java | 23 +++++++++++------ .../procedure/TestModifyTableProcedure.java | 24 +++++++++-------- 3 files changed, 54 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hbase/blob/e989a992/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java index 26b340f..d4951f7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java @@ -34,6 +34,7 @@ import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Predicate; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.ServerName; @@ -570,6 +571,18 @@ public class RegionStates { } /** + * @return Returns regions for a table which are open or about to be open (OPEN or OPENING) + */ + public List<RegionInfo> getOpenRegionsOfTable(final TableName table) { + // We want to get regions which are already open on the cluster or are about to be open. + // The use-case is for identifying regions which need to be re-opened to ensure they see some + // new configuration. Regions in OPENING now are presently being opened by a RS, so we can + // assume that they will imminently be OPEN but may not see our configuration change + return getRegionsOfTable( + table, (state) -> state.isInState(State.OPEN) || state.isInState(State.OPENING)); + } + + /** * @return Return online regions of table; does not include OFFLINE or SPLITTING regions. */ public List<RegionInfo> getRegionsOfTable(final TableName table) { @@ -577,15 +590,25 @@ public class RegionStates { } /** + * @return Return online regions of table; does not include OFFLINE or SPLITTING regions. + */ + public List<RegionInfo> getRegionsOfTable(final TableName table, boolean offline) { + return getRegionsOfTable(table, (state) -> include(state, offline)); + } + + /** * @return Return the regions of the table; does not include OFFLINE unless you set * <code>offline</code> to true. Does not include regions that are in the * {@link State#SPLIT} state. */ - public List<RegionInfo> getRegionsOfTable(final TableName table, final boolean offline) { + public List<RegionInfo> getRegionsOfTable( + final TableName table, Predicate<RegionStateNode> filter) { final ArrayList<RegionStateNode> nodes = getTableRegionStateNodes(table); final ArrayList<RegionInfo> hris = new ArrayList<RegionInfo>(nodes.size()); for (RegionStateNode node: nodes) { - if (include(node, offline)) hris.add(node.getRegionInfo()); + if (filter.test(node)) { + hris.add(node.getRegionInfo()); + } } return hris; } http://git-wip-us.apache.org/repos/asf/hbase/blob/e989a992/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java ---------------------------------------------------------------------- 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 6fb9caa..bcf41ac 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 @@ -56,7 +56,6 @@ public class ModifyTableProcedure private TableDescriptor modifiedTableDescriptor; private boolean deleteColumnFamilyInModify; - private List<RegionInfo> regionInfoList; private Boolean traceEnabled = null; public ModifyTableProcedure() { @@ -80,7 +79,6 @@ public class ModifyTableProcedure private void initilize() { this.unmodifiedTableDescriptor = null; - this.regionInfoList = null; this.traceEnabled = null; this.deleteColumnFamilyInModify = false; } @@ -125,7 +123,7 @@ public class ModifyTableProcedure case MODIFY_TABLE_REOPEN_ALL_REGIONS: if (env.getAssignmentManager().isTableEnabled(getTableName())) { addChildProcedure(env.getAssignmentManager() - .createReopenProcedures(getRegionInfoList(env))); + .createReopenProcedures(getOpenRegionInfoList(env))); } return Flow.NO_MORE_STATE; default: @@ -433,11 +431,20 @@ public class ModifyTableProcedure } } + /** + * Fetches all Regions for a table. Cache the result of this method if you need to use it multiple + * times. Be aware that it may change over in between calls to this procedure. + */ private List<RegionInfo> getRegionInfoList(final MasterProcedureEnv env) throws IOException { - if (regionInfoList == null) { - regionInfoList = env.getAssignmentManager().getRegionStates() - .getRegionsOfTable(getTableName()); - } - return regionInfoList; + return env.getAssignmentManager().getRegionStates().getRegionsOfTable(getTableName()); + } + + /** + * Fetches all open or soon to be open Regions for a table. Cache the result of this method if + * you need to use it multiple times. Be aware that it may change over in between calls to this + * procedure. + */ + private List<RegionInfo> getOpenRegionInfoList(final MasterProcedureEnv env) throws IOException { + return env.getAssignmentManager().getRegionStates().getOpenRegionsOfTable(getTableName()); } } http://git-wip-us.apache.org/repos/asf/hbase/blob/e989a992/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java index c519835..51d7d2b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java @@ -26,7 +26,10 @@ import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HColumnDescriptor; import org.apache.hadoop.hbase.HTableDescriptor; import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.procedure2.Procedure; import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility; @@ -201,24 +204,25 @@ public class TestModifyTableProcedure extends TestTableDDLProcedureBase { ProcedureTestingUtility.setKillAndToggleBeforeStoreUpdate(procExec, true); // Modify multiple properties of the table. - HTableDescriptor htd = new HTableDescriptor(UTIL.getAdmin().getTableDescriptor(tableName)); - boolean newCompactionEnableOption = htd.isCompactionEnabled() ? false : true; - htd.setCompactionEnabled(newCompactionEnableOption); - htd.addFamily(new HColumnDescriptor(cf2)); - htd.removeFamily(Bytes.toBytes(cf3)); - htd.setRegionReplication(3); + TableDescriptor oldDescriptor = UTIL.getAdmin().getDescriptor(tableName); + TableDescriptor newDescriptor = TableDescriptorBuilder.newBuilder(oldDescriptor) + .setCompactionEnabled(!oldDescriptor.isCompactionEnabled()) + .setColumnFamily(ColumnFamilyDescriptorBuilder.of(cf2)) + .removeColumnFamily(Bytes.toBytes(cf3)) + .setRegionReplication(3) + .build(); // Start the Modify procedure && kill the executor long procId = procExec.submitProcedure( - new ModifyTableProcedure(procExec.getEnvironment(), htd)); + new ModifyTableProcedure(procExec.getEnvironment(), newDescriptor)); // Restart the executor and execute the step twice MasterProcedureTestingUtility.testRecoveryAndDoubleExecution(procExec, procId); // Validate descriptor - HTableDescriptor currentHtd = UTIL.getAdmin().getTableDescriptor(tableName); - assertEquals(newCompactionEnableOption, currentHtd.isCompactionEnabled()); - assertEquals(2, currentHtd.getFamiliesKeys().size()); + TableDescriptor currentDescriptor = UTIL.getAdmin().getDescriptor(tableName); + assertEquals(newDescriptor.isCompactionEnabled(), currentDescriptor.isCompactionEnabled()); + assertEquals(2, newDescriptor.getColumnFamilyNames().size()); // cf2 should be added cf3 should be removed MasterProcedureTestingUtility.validateTableCreation(UTIL.getHBaseCluster().getMaster(),
