[GitHub] [hbase] virajjasani commented on a change in pull request #1734: HBASE-24376 MergeNormalizer is merging non-adjacent regions and causi…

[GitBox <gi...@apache.org>]

virajjasani commented on a change in pull request #1734:
URL: https://github.com/apache/hbase/pull/1734#discussion_r427235890



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/MergeNormalizationPlan.java
##########
@@ -69,8 +69,11 @@ public String toString() {
   public void execute(Admin admin) {
     LOG.info("Executing merging normalization plan: " + this);
     try {
+      // Do not use force=true as corner cases can happen, non adjacent regions,
+      // merge with a merged child region with no GC done yet, it is going to
+      // cause all different issues.
       admin.mergeRegionsAsync(firstRegion.getEncodedNameAsBytes(),
-        secondRegion.getEncodedNameAsBytes(), true);
+        secondRegion.getEncodedNameAsBytes(), false);

Review comment:
       Wondering if this is the only change required.

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizerOnCluster.java
##########
@@ -179,6 +182,71 @@ void testRegionNormalizationSplitOnCluster(boolean limitedByQuota) throws Except
     admin.deleteTable(TABLENAME);
   }
 
+  // This test is to make sure that normalizer is only going to merge adjacent regions.
+  @Test
+  @SuppressWarnings("deprecation")
+  public void testNormalizerCannotMergeNonAdjacentRegions() throws Exception {
+    final TableName tableName = TableName.valueOf(name.getMethodName());
+    MiniHBaseCluster cluster = TEST_UTIL.getHBaseCluster();
+    HMaster m = cluster.getMaster();
+
+    // create 5 regions with sizes to trigger merge of small regions
+    final byte[][] keys = {
+      Bytes.toBytes("aa"),
+      Bytes.toBytes("aa1"),
+      Bytes.toBytes("aa1!"),
+      Bytes.toBytes("aa2")
+    };
+
+    try (Table ht = TEST_UTIL.createMultiRegionTable(tableName, new byte[][]{FAMILYNAME}, keys)) {
+      // Need to get sorted list of regions here, the order is
+      // [, "aa"), ["aa", "aa1"), ["aa1", "aa1!"), ["aa1!", "aa2"), ["aa2", )
+      List<HRegion> generatedRegions = TEST_UTIL.getHBaseCluster().getRegions(tableName);
+      Collections.sort(generatedRegions, Comparator.comparing(HRegion::getRegionInfo, RegionInfo.COMPARATOR));
+
+      // Region ["aa", "aa1") and ["aa1!", "aa2") are not adjacent, they are not supposed to
+      // merged.
+      HRegion region = generatedRegions.get(0);
+      generateTestData(region, 3);
+      region.flush(true);
+
+      region = generatedRegions.get(1);
+      generateTestData(region, 1);
+      region.flush(true);
+
+      region = generatedRegions.get(2);
+      generateTestData(region, 3);
+      region.flush(true);
+
+      region = generatedRegions.get(3);
+      generateTestData(region, 1);
+      region.flush(true);
+
+      region = generatedRegions.get(4);
+      generateTestData(region, 5);
+      region.flush(true);
+
+      ModifyableTableDescriptor htd = new ModifyableTableDescriptor(tableName,
+        admin.getDescriptor(tableName));
+
+      htd.setNormalizationEnabled(true);
+      admin.modifyTable(htd);
+
+      admin.flush(tableName);
+
+      assertEquals(5, MetaTableAccessor.getRegionCount(TEST_UTIL.getConnection(), tableName));
+
+      Thread.sleep(5000); // to let region load to update

Review comment:
       Maybe we can do manual server metrics update followed by sleep and then assert null plan?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org