You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by zh...@apache.org on 2019/09/20 04:00:59 UTC

[hbase] 01/02: HBASE-22941 merge operation returns parent regions in random order (#556)

This is an automated email from the ASF dual-hosted git repository.

zhangduo pushed a commit to branch branch-2
in repository https://gitbox.apache.org/repos/asf/hbase.git

commit 908a4d8f2b5b408847c5322a42b578bccf1d7f27
Author: Istvan Toth <st...@users.noreply.github.com>
AuthorDate: Thu Aug 29 21:27:33 2019 +0200

    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 <st...@apache.org>
---
 .../hadoop/hbase/master/MasterRpcServices.java     |  4 --
 .../hbase/master/assignment/RegionStateStore.java  |  6 +--
 .../org/apache/hadoop/hbase/TestSplitMerge.java    | 49 ++++++++++++++++++++++
 .../org/apache/hadoop/hbase/client/TestAdmin1.java | 10 -----
 .../hbase/client/TestAsyncRegionAdminApi2.java     | 11 -----
 5 files changed, 52 insertions(+), 28 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
index fb83bc7..4387462 100644
--- 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
@@ -816,10 +816,6 @@ public class MasterRpcServices extends RSRpcServices
 
     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
index 5a8e7dd..d47433d 100644
--- 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 @@ package org.apache.hadoop.hbase.master.assignment;
 
 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 class RegionStateStore {
       throws IOException {
     TableDescriptor htd = getTableDescriptor(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
index 641e52e..0aa3297 100644
--- 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 @@ package org.apache.hadoop.hbase;
 
 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 class TestSplitMerge {
         .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
index 8b74ec5..4d8f360 100644
--- 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
@@ -28,7 +28,6 @@ import java.util.List;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
-import org.apache.hadoop.hbase.DoNotRetryIOException;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.HRegionLocation;
@@ -581,15 +580,6 @@ public class TestAdmin1 extends TestAdminBase {
       } catch (IllegalArgumentException e) {
         // expected
       }
-      // 3
-      try {
-        ADMIN.mergeRegionsAsync(
-          tableRegions.stream().map(RegionInfo::getEncodedNameAsBytes).toArray(byte[][]::new),
-          false).get();
-        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
index 7d6d2e0..8616ed5 100644
--- 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.Optional;
 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 class TestAsyncRegionAdminApi2 extends TestAsyncAdminBase {
       // 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