From 397135b2b32af6069ed45d882c5ad04b3dc00509 Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Wed, 28 Aug 2019 17:41:14 +0200 Subject: [PATCH 1/4] 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 --- .../hbase/master/MasterRpcServices.java | 4 -- .../master/assignment/RegionStateStore.java | 4 +- .../apache/hadoop/hbase/TestSplitMerge.java | 48 +++++++++++++++++++ 3 files changed, 50 insertions(+), 6 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 e1c21c6e5036..9fd0dc72832b 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 @@ -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 index 2865905dd248..c27a770b81a0 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.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 parentSeqNums = new HashMap<>(parents.length); + Map 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..2ebcf803faad 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,50 @@ 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[20][]; + 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(), 20); + + for (int c = 0; c < 19; c++) { + assertTrue(Bytes.compareTo(mergeParentRegions.get(c).getStartKey(), mergeParentRegions.get(c+1).getStartKey())<0); + } + + } } From 8e8b9cd289bc10fc1860c1e8690314738ce60e0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?T=C3=B3th=20Istv=C3=A1n?= Date: Wed, 28 Aug 2019 20:28:50 +0200 Subject: [PATCH 2/4] use SortedMap type to emphasise that the Map is sorted. --- .../hadoop/hbase/master/assignment/RegionStateStore.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 c27a770b81a0..69bc8f70aa56 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 @@ -20,7 +20,7 @@ import java.io.IOException; import java.util.Collections; import java.util.List; -import java.util.Map; +import java.util.SortedMap; import java.util.TreeMap; import org.apache.hadoop.hbase.Cell; @@ -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 parentSeqNums = new TreeMap<>(); + SortedMap parentSeqNums = new TreeMap<>(); for (RegionInfo ri: parents) { parentSeqNums.put(ri, globalScope? getOpenSeqNumForParentRegion(ri): -1); } From e6c3f92ebbfe258e4911d949c1f1a9b414c8ea75 Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Thu, 29 Aug 2019 07:54:42 +0200 Subject: [PATCH 3/4] use regionCount consistently and checkstyle fixes --- .../java/org/apache/hadoop/hbase/TestSplitMerge.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) 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 2ebcf803faad..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 @@ -128,7 +128,7 @@ public void testMergeRegionOrder() throws Exception { List regions = UTIL.getAdmin().getRegions(tableName); - byte[][] regionNames = new byte[20][]; + byte[][] regionNames = new byte[regionCount][]; for (int c = 0; c < regionCount; c++) { regionNames[c] = regions.get(c).getRegionName(); } @@ -145,11 +145,12 @@ public void testMergeRegionOrder() throws Exception { List mergeParentRegions = MetaTableAccessor.getMergeRegions(UTIL.getConnection(), mergedRegion.getEncodedNameAsBytes()); - assertEquals(mergeParentRegions.size(), 20); + assertEquals(mergeParentRegions.size(), regionCount); - for (int c = 0; c < 19; c++) { - assertTrue(Bytes.compareTo(mergeParentRegions.get(c).getStartKey(), mergeParentRegions.get(c+1).getStartKey())<0); + for (int c = 0; c < regionCount-1; c++) { + assertTrue(Bytes.compareTo( + mergeParentRegions.get(c).getStartKey(), + mergeParentRegions.get(c+1).getStartKey()) < 0); } - } } From 9f5da35437c19e1af892628555a6da45703e12e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?T=C3=B3th=20Istv=C3=A1n?= Date: Thu, 29 Aug 2019 17:46:02 +0200 Subject: [PATCH 4/4] Delete tests that expect multiregion merges to fail. --- .../org/apache/hadoop/hbase/client/TestAdmin1.java | 9 --------- .../hadoop/hbase/client/TestAsyncRegionAdminApi2.java | 11 ----------- 2 files changed, 20 deletions(-) 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 d08c5d461ca4..df496d7cc567 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 @@ -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 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