HBASE-22941 merge operation returns parent regions in random order (#556

) * HBASE-22941 merge operation returns parent regions in random order store and return the merge parent regions in ascending order remove left over check for exactly two merged regions add unit test * use SortedMap type to emphasise that the Map is sorted. * use regionCount consistently and checkstyle fixes * Delete tests that expect multiregion merges to fail. Signed-off-by: stack <stack@apache.org>
apache · Aug 29, 2019 · 56fd3e9 · 56fd3e9
1 parent 83e7794
commit 56fd3e9
Show file tree

Hide file tree

Showing 5 changed files with 52 additions and 27 deletions.
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
@@ -812,10 +812,6 @@ public MergeTableRegionsResponse mergeTableRegions(
 
     RegionStates regionStates = master.getAssignmentManager().getRegionStates();
 
-    if (request.getRegionCount() != 2) {
-      throw new ServiceException(new DoNotRetryIOException(
-        "Only support merging 2 regions but " + request.getRegionCount() + " region passed"));
-    }
     RegionInfo[] regionsToMerge = new RegionInfo[request.getRegionCount()];
     for (int i = 0; i < request.getRegionCount(); i++) {
       final byte[] encodedNameOfRegion = request.getRegion(i).getValue().toByteArray();

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateStore.java
@@ -19,9 +19,9 @@
 
 import java.io.IOException;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
 
 import org.apache.hadoop.hbase.Cell;
 import org.apache.hadoop.hbase.CellBuilderFactory;
@@ -263,7 +263,7 @@ public void mergeRegions(RegionInfo child, RegionInfo [] parents, ServerName ser
       throws IOException {
     TableDescriptor htd = getDescriptor(child.getTable());
     boolean globalScope = htd.hasGlobalReplicationScope();
-    Map<RegionInfo, Long> parentSeqNums = new HashMap<>(parents.length);
+    SortedMap<RegionInfo, Long> parentSeqNums = new TreeMap<>();
     for (RegionInfo ri: parents) {
       parentSeqNums.put(ri, globalScope? getOpenSeqNumForParentRegion(ri): -1);
     }

diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestSplitMerge.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestSplitMerge.java
@@ -19,7 +19,9 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 
+import java.util.List;
 import java.util.concurrent.TimeUnit;
 import org.apache.hadoop.hbase.Waiter.ExplainingPredicate;
 import org.apache.hadoop.hbase.client.AsyncConnection;
@@ -104,4 +106,51 @@ public String explainFailure() throws Exception {
         .getRegionLocation(Bytes.toBytes(1), true).get().getServerName());
     }
   }
+
+  @Test
+  public void testMergeRegionOrder() throws Exception {
+
+    int regionCount= 20;
+
+    TableName tableName = TableName.valueOf("MergeRegionOrder");
+    byte[] family = Bytes.toBytes("CF");
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName)
+        .setColumnFamily(ColumnFamilyDescriptorBuilder.of(family)).build();
+
+    byte[][] splitKeys = new byte[regionCount-1][];
+
+    for (int c = 0; c < regionCount-1; c++) {
+      splitKeys[c] = Bytes.toBytes(c+1 * 1000);
+    }
+
+    UTIL.getAdmin().createTable(td, splitKeys);
+    UTIL.waitTableAvailable(tableName);
+
+    List<RegionInfo> regions = UTIL.getAdmin().getRegions(tableName);
+
+    byte[][] regionNames = new byte[regionCount][];
+    for (int c = 0; c < regionCount; c++) {
+      regionNames[c] = regions.get(c).getRegionName();
+    }
+
+    UTIL.getAdmin().mergeRegionsAsync(regionNames, false).get(60, TimeUnit.SECONDS);
+
+    List<RegionInfo> mergedRegions =
+        MetaTableAccessor.getTableRegions(UTIL.getConnection(), tableName);
+
+    assertEquals(1, mergedRegions.size());
+
+    RegionInfo mergedRegion = mergedRegions.get(0);
+
+    List<RegionInfo> mergeParentRegions = MetaTableAccessor.getMergeRegions(UTIL.getConnection(),
+      mergedRegion.getEncodedNameAsBytes());
+
+    assertEquals(mergeParentRegions.size(), regionCount);
+
+    for (int c = 0; c < regionCount-1; c++) {
+      assertTrue(Bytes.compareTo(
+        mergeParentRegions.get(c).getStartKey(),
+        mergeParentRegions.get(c+1).getStartKey()) < 0);
+    }
+  }
 }
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin1.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin1.java
@@ -588,15 +588,6 @@ public void testMergeRegionsInvalidRegionCount()
       } catch (IllegalArgumentException e) {
         // expected
       }
-      // 3
-      try {
-        FutureUtils.get(ADMIN.mergeRegionsAsync(
-          tableRegions.stream().map(RegionInfo::getEncodedNameAsBytes).toArray(byte[][]::new),
-          false));
-        fail();
-      } catch (DoNotRetryIOException e) {
-        // expected
-      }
     } finally {
       ADMIN.disableTable(tableName);
       ADMIN.deleteTable(tableName);

diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncRegionAdminApi2.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncRegionAdminApi2.java
@@ -32,7 +32,6 @@
 import java.util.concurrent.ExecutionException;
 import java.util.stream.Collectors;
 import org.apache.hadoop.hbase.AsyncMetaTableAccessor;
-import org.apache.hadoop.hbase.DoNotRetryIOException;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.HRegionLocation;
@@ -205,16 +204,6 @@ public void testMergeRegionsInvalidRegionCount() throws Exception {
       // expected
       assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));
     }
-    // 3
-    try {
-      admin.mergeRegions(
-        regions.stream().map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList()), false)
-        .get();
-      fail();
-    } catch (ExecutionException e) {
-      // expected
-      assertThat(e.getCause(), instanceOf(DoNotRetryIOException.class));
-    }
   }
 
   @Test