You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "ndimiduk (via GitHub)" <gi...@apache.org> on 2023/09/19 08:52:23 UTC
[GitHub] [hbase] ndimiduk commented on a diff in pull request #5403: HBASE-28068 : Add hbase.normalizer.merge.max.region.count property …
ndimiduk commented on code in PR #5403:
URL: https://github.com/apache/hbase/pull/5403#discussion_r1329762729
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java:
##########
@@ -81,6 +81,8 @@ class SimpleRegionNormalizer implements RegionNormalizer, ConfigurationObserver
static final int DEFAULT_MERGE_MIN_REGION_AGE_DAYS = 3;
static final String MERGE_MIN_REGION_SIZE_MB_KEY = "hbase.normalizer.merge.min_region_size.mb";
static final int DEFAULT_MERGE_MIN_REGION_SIZE_MB = 0;
+ static final String MERGE_MAX_REGION_COUNT_KEY = "hbase.normalizer.merge.max.region.count";
Review Comment:
However you chance the configuration key, please also change these variable names. As it is, they look identical to the other, subtly-different MAX config variable, and it will be confusing to maintainers.
##########
hbase-common/src/main/resources/hbase-default.xml:
##########
@@ -656,6 +656,12 @@ possible configurations would overwhelm and obscure the important.
<description>The minimum size for a region to be considered for a merge, in whole
MBs.</description>
</property>
+ <property>
+ <name>hbase.normalizer.merge.max.region.count</name>
Review Comment:
Careful here. This config name looks like a counter-point to `hbase.normalizer.merge.min.region.count` but in fact it's not. The `min` config is placing a guard-rail on the minimum number of regions a table contain in order to be considered for merge normalization. This new guard-rail is limiting the number of regions that can be merged together in a single operation.
I instead suggest a config named something like `hbase.normalizer.merge.merge_request_max_number_of_regions`. Also, the description text should be updated accordingly -- this is just a copy-paste of the other config's description.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java:
##########
@@ -382,15 +398,16 @@ private List<NormalizationPlan> computeMergeNormalizationPlans(final NormalizeCo
break;
}
if (
- rangeMembers.isEmpty() // when there are no range members, seed the range with whatever
- // we have. this way we're prepared in case the next region is
- // 0-size.
+ (rangeMembers.isEmpty() // when there are no range members, seed the range with whatever
+ // we have. this way we're prepared in case the next region is
+ // 0-size.
|| (rangeMembers.size() == 1 && sumRangeMembersSizeMb == 0) // when there is only one
// region and the size is 0,
// seed the range with
// whatever we have.
|| regionSizeMb == 0 // always add an empty region to the current range.
- || (regionSizeMb + sumRangeMembersSizeMb <= avgRegionSizeMb)
+ || (regionSizeMb + sumRangeMembersSizeMb <= avgRegionSizeMb))
+ && (rangeMembers.size() < getMergeMaxRegionCount())
Review Comment:
Please invert the logic to be another `|| condition` -- adding the `&&` makes the logic more confusing.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java:
##########
@@ -382,15 +398,16 @@ private List<NormalizationPlan> computeMergeNormalizationPlans(final NormalizeCo
break;
}
if (
- rangeMembers.isEmpty() // when there are no range members, seed the range with whatever
- // we have. this way we're prepared in case the next region is
- // 0-size.
+ (rangeMembers.isEmpty() // when there are no range members, seed the range with whatever
Review Comment:
While you're here, do you mind cleaning up these comment strings? They're awkwardly broken over several lines, probably because of the spotless:apply automation. Something like,
```
if (
// when there are no range members, seed the range with whatever we have. this way we're
// prepared in case the next region is0-size.
rangeMembers.isEmpty()
// when there is only one region and the size is 0, seed the range with whatever we
// have.
|| (rangeMembers.size() == 1 && sumRangeMembersSizeMb == 0)
// always add an empty region to the current range.
|| regionSizeMb == 0
|| (regionSizeMb + sumRangeMembersSizeMb <= avgRegionSizeMb)
) {
// add the current region to the range when there's capacity remaining.
rangeMembers.add(new NormalizationTarget(regionInfo, regionSizeMb));
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java:
##########
@@ -138,6 +140,16 @@ private static long parseMergeMinRegionSizeMb(final Configuration conf) {
return settledValue;
}
+ private static long parseMergeMaxRegionCount(final Configuration conf) {
+ final long parsedValue =
+ conf.getLong(MERGE_MAX_REGION_COUNT_KEY, DEFAULT_MERGE_MAX_REGION_COUNT);
+ final long settledValue = Math.max(1, parsedValue);
Review Comment:
The minimum value should be 2 ; it makes no sense to merge a single region.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java:
##########
@@ -81,6 +81,8 @@ class SimpleRegionNormalizer implements RegionNormalizer, ConfigurationObserver
static final int DEFAULT_MERGE_MIN_REGION_AGE_DAYS = 3;
static final String MERGE_MIN_REGION_SIZE_MB_KEY = "hbase.normalizer.merge.min_region_size.mb";
static final int DEFAULT_MERGE_MIN_REGION_SIZE_MB = 0;
+ static final String MERGE_MAX_REGION_COUNT_KEY = "hbase.normalizer.merge.max.region.count";
+ static final long DEFAULT_MERGE_MAX_REGION_COUNT = Long.MAX_VALUE;
Review Comment:
Better to put a large but limiting default, so that no one else hits the same problem you do, right? In order to execute a merge procedure, the master must assign all involved regions to the same region server. So likely a majority of the regions involved have to be moved first -- all those assign and close and open procedures. How about a default of 100? 1_000?
Remember, this limits the size of a single merge request. With a default of 100, your problematic situation with 25k regions would still generate 250 merge procedures (much better, but still a lot all at once).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org