HBASE-24995: MetaFixer fails to fix overlaps when multiple tables hav…

…e overlaps (#2361)

Signed-off-by: stack <[email protected]>

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

@@ -19,9 +19,11 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.SortedSet;
@@ -39,6 +41,8 @@
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.util.Pair;
 import org.apache.hadoop.hbase.util.ServerRegionReplicaUtil;
+import org.apache.hbase.thirdparty.com.google.common.collect.ArrayListMultimap;
+import org.apache.hbase.thirdparty.com.google.common.collect.ListMultimap;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -231,16 +235,18 @@ private static List<RegionInfo> createMetaEntries(final MasterServices masterSer
   /**
    * Fix overlaps noted in CJ consistency report.
    */
-  void fixOverlaps(CatalogJanitor.Report report) throws IOException {
+  List<Long> fixOverlaps(CatalogJanitor.Report report) throws IOException {
+    List<Long> pidList = new ArrayList<>();
     for (Set<RegionInfo> regions: calculateMerges(maxMergeCount, report.getOverlaps())) {
       RegionInfo [] regionsArray = regions.toArray(new RegionInfo [] {});
       try {
-        this.masterServices.mergeRegions(regionsArray,
-            true, HConstants.NO_NONCE, HConstants.NO_NONCE);
+        pidList.add(this.masterServices
+          .mergeRegions(regionsArray, true, HConstants.NO_NONCE, HConstants.NO_NONCE));
       } catch (MergeRegionException mre) {
         LOG.warn("Failed overlap fix of {}", regionsArray, mre);
       }
     }
+    return pidList;
   }
 
   /**
@@ -258,6 +264,21 @@ static List<SortedSet<RegionInfo>> calculateMerges(int maxMergeCount,
       return Collections.emptyList();
     }
     List<SortedSet<RegionInfo>> merges = new ArrayList<>();
+    // First group overlaps by table then calculate merge table by table.
+    ListMultimap<TableName, Pair<RegionInfo, RegionInfo>> overlapGroups =
+      ArrayListMultimap.create();
+    for (Pair<RegionInfo, RegionInfo> pair : overlaps) {
+      overlapGroups.put(pair.getFirst().getTable(), pair);
+    }
+    for (Map.Entry<TableName, Collection<Pair<RegionInfo, RegionInfo>>> entry : overlapGroups
+      .asMap().entrySet()) {
+      calculateTableMerges(maxMergeCount, merges, entry.getValue());
+    }
+    return merges;
+  }
+
+  private static void calculateTableMerges(int maxMergeCount, List<SortedSet<RegionInfo>> merges,
+    Collection<Pair<RegionInfo, RegionInfo>> overlaps) {
     SortedSet<RegionInfo> currentMergeSet = new TreeSet<>();
     HashSet<RegionInfo> regionsInMergeSet = new HashSet<>();
     RegionInfo regionInfoWithlargestEndKey =  null;
@@ -301,7 +322,6 @@ static List<SortedSet<RegionInfo>> calculateMerges(int maxMergeCount,
           regionInfoWithlargestEndKey);
     }
     merges.add(currentMergeSet);
-    return merges;
   }
 
   /**

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

@@ -46,6 +46,7 @@
 import org.apache.hadoop.hbase.master.assignment.RegionStates;
 import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
+import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility;
 import org.apache.hadoop.hbase.testclassification.LargeTests;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
 import org.apache.hadoop.hbase.util.Bytes;
@@ -171,7 +172,8 @@ private static RegionInfo makeOverlap(MasterServices services, RegionInfo a, Reg
         Collections.singletonList(MetaTableAccessor.makePutFromRegionInfo(overlapRegion,
             System.currentTimeMillis())));
     // TODO: Add checks at assign time to PREVENT being able to assign over existing assign.
-    services.getAssignmentManager().assign(overlapRegion);
+    long assign = services.getAssignmentManager().assign(overlapRegion);
+    ProcedureTestingUtility.waitProcedures(services.getMasterProcedureExecutor(), assign);
     return overlapRegion;
   }
 
@@ -244,6 +246,41 @@ HBaseTestingUtility. await(10, () -> {
     assertTrue(postReport.isEmpty());
   }
 
+  @Test
+  public void testMultipleTableOverlaps() throws Exception {
+    TableName t1 = TableName.valueOf("t1");
+    TableName t2 = TableName.valueOf("t2");
+    TEST_UTIL.createMultiRegionTable(t1, new byte[][] { HConstants.CATALOG_FAMILY });
+    TEST_UTIL.createMultiRegionTable(t2, new byte[][] { HConstants.CATALOG_FAMILY });
+    TEST_UTIL.waitTableAvailable(t2);
+
+    HMaster services = TEST_UTIL.getHBaseCluster().getMaster();
+    services.getCatalogJanitor().scan();
+    CatalogJanitor.Report report = services.getCatalogJanitor().getLastReport();
+    assertTrue(report.isEmpty());
+
+    // Make a simple overlap for t1
+    List<RegionInfo> ris = MetaTableAccessor.getTableRegions(TEST_UTIL.getConnection(), t1);
+    makeOverlap(services, ris.get(1), ris.get(2));
+    // Make a simple overlap for t2
+    ris = MetaTableAccessor.getTableRegions(TEST_UTIL.getConnection(), t2);
+    makeOverlap(services, ris.get(1), ris.get(2));
+
+    services.getCatalogJanitor().scan();
+    report = services.getCatalogJanitor().getLastReport();
+    assertEquals("Region overlaps count does not match.", 4, report.getOverlaps().size());
+
+    MetaFixer fixer = new MetaFixer(services);
+    List<Long> longs = fixer.fixOverlaps(report);
+    long[] procIds = longs.stream().mapToLong(l -> l).toArray();
+    ProcedureTestingUtility.waitProcedures(services.getMasterProcedureExecutor(), procIds);
+
+    // After fix, verify no overlaps are left.
+    services.getCatalogJanitor().scan();
+    report = services.getCatalogJanitor().getLastReport();
+    assertTrue("After fix there should not have been any overlaps.", report.isEmpty());
+  }
+
   @Test
   public void testOverlapWithSmallMergeCount() throws Exception {
     TableName tn = TableName.valueOf(this.name.getMethodName());