HBASE-24705 MetaFixer#fixHoles() does not include the case for read replicas (i.e, replica regions are not created) (#2062)

Signed-off-by: Viraj Jasani <vjasani@apache.org>

hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionReplicaUtil.java

@@ -147,14 +147,13 @@ public class RegionReplicaUtil {
   /**
    * Create any replicas for the regions (the default replicas that was already created is passed to
    * the method)
-   * @param tableDescriptor descriptor to use
    * @param regions existing regions
    * @param oldReplicaCount existing replica count
    * @param newReplicaCount updated replica count due to modify table
    * @return the combined list of default and non-default replicas
    */
-  public static List<RegionInfo> addReplicas(final TableDescriptor tableDescriptor,
+  public static List<RegionInfo> addReplicas(final List<RegionInfo> regions, int oldReplicaCount,
-      final List<RegionInfo> regions, int oldReplicaCount, int newReplicaCount) {
+    int newReplicaCount) {
     if ((newReplicaCount - 1) <= 0) {
       return regions;
     }

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

@@ -32,10 +32,13 @@ import org.apache.hadoop.hbase.MetaTableAccessor;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.RegionInfo;
 import org.apache.hadoop.hbase.client.RegionInfoBuilder;
+import org.apache.hadoop.hbase.client.RegionReplicaUtil;
+import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.hbase.exceptions.MergeRegionException;
 import org.apache.hadoop.hbase.master.assignment.TransitRegionStateProcedure;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.util.Pair;
+import org.apache.hadoop.hbase.util.ServerRegionReplicaUtil;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -174,22 +177,35 @@ class MetaFixer {
   private static List<RegionInfo> createMetaEntries(final MasterServices masterServices,
     final List<RegionInfo> newRegionInfos) {
 
-    final List<Either<RegionInfo, IOException>> addMetaEntriesResults = newRegionInfos.stream()
+    final List<Either<List<RegionInfo>, IOException>> addMetaEntriesResults = newRegionInfos.
-      .map(regionInfo -> {
+      stream().map(regionInfo -> {
         try {
-          MetaTableAccessor.addRegionToMeta(masterServices.getConnection(), regionInfo);
+          TableDescriptor td = masterServices.getTableDescriptors().get(regionInfo.getTable());
-          masterServices.getAssignmentManager()
+
-            .getRegionStates()
+          // Add replicas if needed
-            .updateRegionState(regionInfo, RegionState.State.CLOSED);
+          // we need to create regions with replicaIds starting from 1
-          return Either.<RegionInfo, IOException>ofLeft(regionInfo);
+          List<RegionInfo> newRegions = RegionReplicaUtil.addReplicas(
+            Collections.singletonList(regionInfo), 1, td.getRegionReplication());
+
+          // Add regions to META
+          MetaTableAccessor.addRegionsToMeta(masterServices.getConnection(), newRegions,
+            td.getRegionReplication());
+
+          // Setup replication for region replicas if needed
+          if (td.getRegionReplication() > 1) {
+            ServerRegionReplicaUtil.setupRegionReplicaReplication(
+              masterServices.getConfiguration());
+          }
+          return Either.<List<RegionInfo>, IOException>ofLeft(newRegions);
         } catch (IOException e) {
-          return Either.<RegionInfo, IOException>ofRight(e);
+          return Either.<List<RegionInfo>, IOException>ofRight(e);
         }
       })
       .collect(Collectors.toList());
     final List<RegionInfo> createMetaEntriesSuccesses = addMetaEntriesResults.stream()
       .filter(Either::hasLeft)
       .map(Either::getLeft)
+      .flatMap(List::stream)
       .collect(Collectors.toList());
     final List<IOException> createMetaEntriesFailures = addMetaEntriesResults.stream()
       .filter(Either::hasRight)

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

@@ -369,7 +369,7 @@ public class CreateTableProcedure
 
     // Add replicas if needed
     // we need to create regions with replicaIds starting from 1
-    List<RegionInfo> newRegions = RegionReplicaUtil.addReplicas(tableDescriptor, regions, 1,
+    List<RegionInfo> newRegions = RegionReplicaUtil.addReplicas(regions, 1,
       tableDescriptor.getRegionReplication());
 
     // Add regions to META

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

@@ -145,7 +145,7 @@ public class EnableTableProcedure
             LOG.info("Number of replicas has increased. Assigning new region replicas." +
                 "The previous replica count was {}. The current replica count is {}.",
                 (currentMaxReplica + 1), configuredReplicaCount);
-            regionsOfTable = RegionReplicaUtil.addReplicas(tableDescriptor, regionsOfTable,
+            regionsOfTable = RegionReplicaUtil.addReplicas(regionsOfTable,
               currentMaxReplica + 1, configuredReplicaCount);
           }
           // Assign all the table regions. (including region replicas if added).

hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java

@@ -1530,6 +1530,19 @@ public class HBaseTestingUtility extends HBaseZKTestingUtility {
     return createTable(tableName, families, KEYS_FOR_HBA_CREATE_TABLE);
   }
 
+  /**
+   * Create a table with multiple regions.
+   * @param tableName
+   * @param replicaCount replica count.
+   * @param families
+   * @return A Table instance for the created table.
+   * @throws IOException
+   */
+  public Table createMultiRegionTable(TableName tableName, int replicaCount, byte[][] families)
+    throws IOException {
+    return createTable(tableName, families, KEYS_FOR_HBA_CREATE_TABLE, replicaCount);
+  }
+
   /**
    * Create a table.
    * @param tableName

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

@@ -83,10 +83,9 @@ public class TestMetaFixer {
     services.getAssignmentManager().getRegionStates().deleteRegion(ri);
   }
 
-  @Test
+  private void testPlugsHolesWithReadReplicaInternal(final TableName tn, final int replicaCount)
-  public void testPlugsHoles() throws Exception {
+    throws Exception {
-    TableName tn = TableName.valueOf(this.name.getMethodName());
+    TEST_UTIL.createMultiRegionTable(tn, replicaCount, new byte[][] { HConstants.CATALOG_FAMILY });
-    TEST_UTIL.createMultiRegionTable(tn, HConstants.CATALOG_FAMILY);
     List<RegionInfo> ris = MetaTableAccessor.getTableRegions(TEST_UTIL.getConnection(), tn);
     MasterServices services = TEST_UTIL.getHBaseCluster().getMaster();
     int initialSize = services.getAssignmentManager().getRegionStates().getRegionStates().size();
@@ -94,12 +93,14 @@ public class TestMetaFixer {
     CatalogJanitor.Report report = services.getCatalogJanitor().getLastReport();
     assertTrue(report.isEmpty());
     int originalCount = ris.size();
-    // Remove first, last and middle region. See if hole gets plugged. Table has 26 regions.
+    // Remove first, last and middle region. See if hole gets plugged. Table has 26 * replicaCount regions.
-    deleteRegion(services, ris.get(ris.size() -1));
+    for (int i = 0; i < replicaCount; i ++) {
-    deleteRegion(services, ris.get(3));
+      deleteRegion(services, ris.get(3 * replicaCount + i));
-    deleteRegion(services, ris.get(0));
+      deleteRegion(services, ris.get(i));
-    assertEquals(initialSize - 3,
+      deleteRegion(services, ris.get(ris.size() - 1 - i));
-        services.getAssignmentManager().getRegionStates().getRegionStates().size());
+    }
+    assertEquals(initialSize - 3 * replicaCount,
+      services.getAssignmentManager().getRegionStates().getRegionStates().size());
     services.getCatalogJanitor().scan();
     report = services.getCatalogJanitor().getLastReport();
     assertEquals(report.toString(), 3, report.getHoles().size());
@@ -109,17 +110,29 @@ public class TestMetaFixer {
     report = services.getCatalogJanitor().getLastReport();
     assertTrue(report.toString(), report.isEmpty());
     assertEquals(initialSize,
-        services.getAssignmentManager().getRegionStates().getRegionStates().size());
+      services.getAssignmentManager().getRegionStates().getRegionStates().size());
 
     // wait for RITs to settle -- those are the fixed regions being assigned -- or until the
     // watchdog TestRule terminates the test.
     HBaseTestingUtility.await(50,
-      () -> isNotEmpty(services.getAssignmentManager().getRegionsInTransition()));
+      () -> services.getMasterProcedureExecutor().getActiveProcIds().size() == 0);
 
     ris = MetaTableAccessor.getTableRegions(TEST_UTIL.getConnection(), tn);
     assertEquals(originalCount, ris.size());
   }
 
+  @Test
+  public void testPlugsHoles() throws Exception {
+    TableName tn = TableName.valueOf(this.name.getMethodName());
+    testPlugsHolesWithReadReplicaInternal(tn, 1);
+  }
+
+  @Test
+  public void testPlugsHolesWithReadReplica() throws Exception {
+    TableName tn = TableName.valueOf(this.name.getMethodName());
+    testPlugsHolesWithReadReplicaInternal(tn, 3);
+  }
+
   /**
    * Just make sure running fixMeta does right thing for the case
    * of a single-region Table where the region gets dropped.