HBASE-25207 Revisit the implementation and usage of RegionStates.incl…

…ude (#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 <brfrn169@gmail.com>

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -72,7 +72,6 @@
 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;

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java

@@ -874,8 +874,13 @@ private TransitRegionStateProcedure[] createAssignProcedures(
   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 void markRegionAsSplit(final RegionInfo parent, final ServerName serverNa
     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();

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java

@@ -153,7 +153,7 @@ public void deleteRegions(final List<RegionInfo> regionInfos) {
     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 boolean hasTableRegionStates(final TableName tableName) {
   /**
    * @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 HRegionLocation checkReopened(HRegionLocation oldLoc) {
   }
 
   /**
-   * @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 @@ private List<RegionInfo> getRegionsOfTable(TableName table, Predicate<RegionStat
    * @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());
     }

hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java

@@ -97,9 +97,9 @@ protected Flow executeFromState(final MasterProcedureEnv env, final EnableTableS
           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.

hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionStates.java

@@ -58,15 +58,15 @@ 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 {
     threadPool = Threads.getBoundedCachedThreadPool(32, 60L, TimeUnit.SECONDS,
       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 void testRegionDoubleCreation() throws Exception {
     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 Object call() {
     });
   }
 
-  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))