You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by zh...@apache.org on 2020/10/22 14:24:32 UTC
[hbase] branch branch-2 updated: HBASE-25207 Revisit the
implementation and usage of RegionStates.include (#2571)
This is an automated email from the ASF dual-hosted git repository.
zhangduo pushed a commit to branch branch-2
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2 by this push:
new fdea847 HBASE-25207 Revisit the implementation and usage of RegionStates.include (#2571)
fdea847 is described below
commit fdea8471303bf60165d17eb8735dc01ba76dde2c
Author: Duo Zhang <zh...@apache.org>
AuthorDate: Thu Oct 22 21:50:43 2020 +0800
HBASE-25207 Revisit the implementation and usage of RegionStates.include (#2571)
Remove the RegionStates.include method as its name is ambiguous.
Add more comments to describe the logic on why we filter region like
this.
Signed-off-by: Toshihiro Suzuki <br...@gmail.com>
---
.../org/apache/hadoop/hbase/master/HMaster.java | 1 -
.../hbase/master/assignment/AssignmentManager.java | 17 +++++++++++--
.../hbase/master/assignment/RegionStates.java | 28 ++++++++++++++--------
.../master/procedure/EnableTableProcedure.java | 4 ++--
.../hbase/master/assignment/TestRegionStates.java | 23 +++++++-----------
5 files changed, 44 insertions(+), 29 deletions(-)
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 461601a..719bebb 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
@@ -72,7 +72,6 @@ import org.apache.hadoop.hbase.HBaseInterfaceAudience;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.InvalidFamilyOperationException;
import org.apache.hadoop.hbase.MasterNotRunningException;
-import org.apache.hadoop.hbase.MetaTableAccessor;
import org.apache.hadoop.hbase.NamespaceDescriptor;
import org.apache.hadoop.hbase.PleaseHoldException;
import org.apache.hadoop.hbase.ReplicationPeerNotFoundException;
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 fbb29e0..e3bbf2e 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
@@ -874,8 +874,13 @@ public class AssignmentManager {
private TransitRegionStateProcedure forceCreateUnssignProcedure(RegionStateNode regionNode) {
regionNode.lock();
try {
- if (!regionStates.include(regionNode, false) ||
- regionStates.isRegionOffline(regionNode.getRegionInfo())) {
+ if (regionNode.isInState(State.OFFLINE, State.CLOSED, State.SPLIT)) {
+ return null;
+ }
+ // in general, a split parent should be in CLOSED or SPLIT state, but anyway, let's check it
+ // here for safety
+ if (regionNode.getRegionInfo().isSplit()) {
+ LOG.warn("{} is a split parent but not in CLOSED or SPLIT state", regionNode);
return null;
}
// As in DisableTableProcedure or ModifyTableProcedure, we will hold the xlock for table, so
@@ -1906,6 +1911,14 @@ public class AssignmentManager {
final RegionStateNode nodeB = regionStates.getOrCreateRegionStateNode(daughterB);
nodeB.setState(State.SPLITTING_NEW);
+ // TODO: here we just update the parent region info in meta, to set split and offline to true,
+ // without changing the one in the region node. This is a bit confusing but the region info
+ // field in RegionStateNode is not expected to be changed in the current design. Need to find a
+ // possible way to address this problem, or at least adding more comments about the trick to
+ // deal with this problem, that when you want to filter out split parent, you need to check both
+ // the RegionState on whether it is split, and also the region info. If one of them matches then
+ // it is a split parent. And usually only one of them can match, as after restart, the region
+ // state will be changed from SPLIT to CLOSED.
regionStateStore.splitRegion(parent, daughterA, daughterB, serverName);
if (shouldAssignFavoredNodes(parent)) {
List<ServerName> onlineServers = this.master.getServerManager().getOnlineServersList();
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 8d1593a..84f32fc 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
@@ -153,7 +153,7 @@ public class RegionStates {
regionInfos.forEach(this::deleteRegion);
}
- ArrayList<RegionStateNode> getTableRegionStateNodes(final TableName tableName) {
+ List<RegionStateNode> getTableRegionStateNodes(final TableName tableName) {
final ArrayList<RegionStateNode> regions = new ArrayList<RegionStateNode>();
for (RegionStateNode node: regionsMap.tailMap(tableName.getName()).values()) {
if (!node.getTable().equals(tableName)) break;
@@ -223,8 +223,10 @@ public class RegionStates {
/**
* @return Return online regions of table; does not include OFFLINE or SPLITTING regions.
*/
- public List<RegionInfo> getRegionsOfTable(final TableName table) {
- return getRegionsOfTable(table, false);
+ public List<RegionInfo> getRegionsOfTable(TableName table) {
+ return getRegionsOfTable(table,
+ regionNode -> !regionNode.isInState(State.OFFLINE, State.SPLIT) &&
+ !regionNode.getRegionInfo().isSplitParent());
}
private HRegionLocation createRegionForReopen(RegionStateNode node) {
@@ -328,16 +330,22 @@ public class RegionStates {
}
/**
- * @return Return online regions of table; does not include OFFLINE or SPLITTING regions.
+ * Get the regions for enabling a table.
+ * <p/>
+ * Here we want the EnableTableProcedure to be more robust and can be used to fix some nasty
+ * states, so the checks in this method will be a bit strange. In general, a region can only be
+ * offline when it is split, for merging we will just delete the parent regions, but with HBCK we
+ * may force update the state of a region to fix some nasty bugs, so in this method we will try to
+ * bring the offline regions back if it is not split. That's why we only check for split state
+ * here.
*/
- public List<RegionInfo> getRegionsOfTable(TableName table, boolean offline) {
- return getRegionsOfTable(table, state -> include(state, offline));
+ public List<RegionInfo> getRegionsOfTableForEnabling(TableName table) {
+ return getRegionsOfTable(table,
+ regionNode -> !regionNode.isInState(State.SPLIT) && !regionNode.getRegionInfo().isSplit());
}
/**
- * @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.
+ * @return Return the regions of the table and filter them.
*/
private List<RegionInfo> getRegionsOfTable(TableName table, Predicate<RegionStateNode> filter) {
return getTableRegionStateNodes(table).stream().filter(filter).map(n -> n.getRegionInfo())
@@ -350,7 +358,7 @@ public class RegionStates {
* @return True if we should include the <code>node</code> (do not include
* if split or offline unless <code>offline</code> is set to true.
*/
- boolean include(final RegionStateNode node, final boolean offline) {
+ private boolean include(final RegionStateNode node, final boolean offline) {
if (LOG.isTraceEnabled()) {
LOG.trace("WORKING ON " + node + " " + node.getRegionInfo());
}
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 1e48981..8b295ec 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
@@ -97,9 +97,9 @@ public class EnableTableProcedure
TableDescriptor tableDescriptor =
env.getMasterServices().getTableDescriptors().get(tableName);
int configuredReplicaCount = tableDescriptor.getRegionReplication();
- // Get regions for the table from memory; get both online and offline regions ('true').
+ // Get regions for the table from memory
List<RegionInfo> regionsOfTable =
- env.getAssignmentManager().getRegionStates().getRegionsOfTable(tableName, true);
+ env.getAssignmentManager().getRegionStates().getRegionsOfTableForEnabling(tableName);
// How many replicas do we currently have? Check regions returned from
// in-memory state.
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionStates.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionStates.java
index 48cca30..b24ec16 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionStates.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionStates.java
@@ -58,7 +58,7 @@ public class TestRegionStates {
protected static final HBaseTestingUtility UTIL = new HBaseTestingUtility();
private static ThreadPoolExecutor threadPool;
- private static ExecutorCompletionService executorService;
+ private static ExecutorCompletionService<Object> executorService;
@BeforeClass
public static void setUp() throws Exception {
@@ -66,7 +66,7 @@ public class TestRegionStates {
new ThreadFactoryBuilder().setNameFormat("ProcedureDispatcher-pool-%d").setDaemon(true)
.setUncaughtExceptionHandler((t, e) -> LOG.warn("Failed thread " + t.getName(), e))
.build());
- executorService = new ExecutorCompletionService(threadPool);
+ executorService = new ExecutorCompletionService<>(threadPool);
}
@AfterClass
@@ -129,13 +129,13 @@ public class TestRegionStates {
checkTableRegions(stateMap, TABLE_NAME_C, NSMALL_RUNS);
}
- private void checkTableRegions(final RegionStates stateMap,
- final TableName tableName, final int nregions) {
- List<RegionInfo> hris = stateMap.getRegionsOfTable(tableName, true);
- assertEquals(nregions, hris.size());
- for (int i = 1; i < hris.size(); ++i) {
- long a = Bytes.toLong(hris.get(i - 1).getStartKey());
- long b = Bytes.toLong(hris.get(i + 0).getStartKey());
+ private void checkTableRegions(final RegionStates stateMap, final TableName tableName,
+ final int nregions) {
+ List<RegionStateNode> rns = stateMap.getTableRegionStateNodes(tableName);
+ assertEquals(nregions, rns.size());
+ for (int i = 1; i < rns.size(); ++i) {
+ long a = Bytes.toLong(rns.get(i - 1).getRegionInfo().getStartKey());
+ long b = Bytes.toLong(rns.get(i + 0).getRegionInfo().getStartKey());
assertEquals(b, a + 1);
}
}
@@ -155,11 +155,6 @@ public class TestRegionStates {
});
}
- private Object createRegionNode(final RegionStates stateMap,
- final TableName tableName, final long regionId) {
- return stateMap.getOrCreateRegionStateNode(createRegionInfo(tableName, regionId));
- }
-
private RegionInfo createRegionInfo(final TableName tableName, final long regionId) {
return RegionInfoBuilder.newBuilder(tableName)
.setStartKey(Bytes.toBytes(regionId))