From 047aad1e84775b3e75636b2780af775ffc9e4cba Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Thu, 29 Aug 2019 21:27:33 +0200 Subject: [PATCH] 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 --- .../hbase/master/MasterRpcServices.java | 4 -- .../master/assignment/RegionStateStore.java | 6 +-- .../apache/hadoop/hbase/TestSplitMerge.java | 49 +++++++++++++++++++ .../hadoop/hbase/client/TestAdmin1.java | 10 ---- .../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 5823fa785cca..2ff036ce0783 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 @@ -805,10 +805,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 index fc88a75ffe11..dcc38c827d8a 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 @@ 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; @@ -246,7 +246,7 @@ public void mergeRegions(RegionInfo child, RegionInfo [] parents, ServerName ser throws IOException { TableDescriptor htd = getTableDescriptor(child.getTable()); boolean globalScope = htd.hasGlobalReplicationScope(); - Map parentSeqNums = new HashMap<>(parents.length); + SortedMap 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 641e52e1155b..0aa329784ed6 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 @@ 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 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 mergedRegions = + MetaTableAccessor.getTableRegions(UTIL.getConnection(), tableName); + + assertEquals(1, mergedRegions.size()); + + RegionInfo mergedRegion = mergedRegions.get(0); + + List 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 bbff17a2685a..c552fe60cf57 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 @@ -32,7 +32,6 @@ 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.HBaseTestingUtility; import org.apache.hadoop.hbase.HColumnDescriptor; @@ -1467,15 +1466,6 @@ public void testMergeRegionsInvalidRegionCount() } 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 7d6d2e093e5c..8616ed565b55 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.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