You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/05/18 22:07:22 UTC
[GitHub] [hbase] ndimiduk commented on a change in pull request #1734: HBASE-24376 MergeNormalizer is merging non-adjacent regions and causi…
ndimiduk commented on a change in pull request #1734:
URL: https://github.com/apache/hbase/pull/1734#discussion_r426915633
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/AbstractRegionNormalizer.java
##########
@@ -170,6 +170,8 @@ private static boolean skipForMerge(final RegionState state) {
+ "number of regions: {}",
table, avgRegionSize, table, tableRegions.size());
+ // Sorting based on RegionInfo.COMPARATOR.
+ tableRegions.sort(RegionInfo.COMPARATOR);
Review comment:
This should be unnecessary. Why does the `AssignmentManager` not provide them in sorted order? I guess it doesn't hurt anything to check.. just seems redundant.
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
##########
@@ -1519,6 +1519,17 @@ public Table createMultiRegionTable(TableName tableName, byte[][] families) thro
return createTable(tableName, families, KEYS_FOR_HBA_CREATE_TABLE);
}
+ /**
+ * Create a table with multiple regions.
+ * @param tableName
+ * @param families
+ * @return A Table instance for the created table.
+ * @throws IOException
+ */
+ public Table createMultiRegionTable(TableName tableName, byte[][] families, final byte[][] keys) throws IOException {
Review comment:
This method does nothing at all. Is it necessary? Does it help code readability so much that it's worth adding?
##########
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
+
+ // Compute the plan, no merge plan returned as they are not adjacent.
+ final List<NormalizationPlan> plans = m.getRegionNormalizer().computePlanForTable(tableName);
+ assertNull(plans);
+ } finally {
+ admin.disableTable(tableName);
Review comment:
I think this should be wrapped in `if admin.exists(tableName)`, otherwise flakey test.
##########
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:
Is there a condition you can `waitFor` instead of a fixed sleep? These fixed sleeps lead to slow and flakey tests :(
##########
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")
Review comment:
There's a new API you can use instead of this deprecated one. I think you want something like
```java
final TableDescriptor td = TableDescriptorBuilder
.newBuilder(admin.getDescriptor(TABLENAME))
.setNormalizationEnabled(true)
.build();
admin.modifyTable(td);
```
----------------------------------------------------------------
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