Skip to content

HBASE-27496 Optionally limit the cumulative size of normalization plans produced by SimpleRegionNormalizer #4888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bbeaudreault merged 6 commits into from
Nov 22, 2022
Merged

HBASE-27496 Optionally limit the cumulative size of normalization plans produced by SimpleRegionNormalizer #4888

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b169ee8
Optionally limit the cumulative size of normalization plans produced …
charlesconnell Nov 16, 2022
ea41bf2
Use a single setting to limit normalizer plans size
charlesconnell Nov 19, 2022
7c2025b
PR feedback: move plans truncation into RegionNormalizerWorker
charlesconnell Nov 21, 2022
f29755f
Add logging
charlesconnell Nov 21, 2022
bd0ad01
Log more information
charlesconnell Nov 21, 2022
0aad517
shorter line
charlesconnell Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...erver/src/main/java/org/apache/hadoop/hbase/master/normalizer/MergeNormalizationPlan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ public List<NormalizationTarget> getNormalizationTargets() {
return normalizationTargets;
}

@Override
public long getPlanSizeMb() {
long total = 0;
for (NormalizationTarget target : normalizationTargets) {
total += target.getRegionSizeMb();
}
return total;
}

@Override
public String toString() {
return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)
Expand Down
2 changes: 2 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/NormalizationPlan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,6 @@ enum PlanType {

/** Returns the type of this plan */
PlanType getType();

long getPlanSizeMb();
}
37 changes: 36 additions & 1 deletion ...erver/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,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 CUMULATIVE_SIZE_LIMIT_MB_KEY = "hbase.normalizer.plans_size_limit.mb";
Comment thread
bbeaudreault marked this conversation as resolved.
Outdated
static final long DEFAULT_CUMULATIVE_SIZE_LIMIT_MB = Long.MAX_VALUE;

private MasterServices masterServices;
private NormalizerConfiguration normalizerConfiguration;
Expand Down Expand Up @@ -215,7 +217,7 @@ public List<NormalizationPlan> computePlansForTable(final TableDescriptor tableD
LOG.debug("Computing normalization plan for table: {}, number of regions: {}", table,
ctx.getTableRegions().size());

final List<NormalizationPlan> plans = new ArrayList<>();
List<NormalizationPlan> plans = new ArrayList<>();
int splitPlansCount = 0;
if (proceedWithSplitPlanning) {
List<NormalizationPlan> splitPlans = computeSplitNormalizationPlans(ctx);
Expand All @@ -229,6 +231,8 @@ public List<NormalizationPlan> computePlansForTable(final TableDescriptor tableD
plans.addAll(mergePlans);
}

plans = truncateForSize(plans);

@bbeaudreault bbeaudreault Nov 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only other thought is whether it makes sense to do the truncating in the NormalizationWorker, like how it currently handles rate limiting

@charlesconnell charlesconnell Nov 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference there is that the new setting would apply to HBase clusters using alternate RegionNormalizer implementations. I really don't know if that's a pro or a con.

@bbeaudreault bbeaudreault Nov 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, to me the separation of concerns makes sense -- RegionNormalizer generates plans, RegionNormalizerWorker executes those plans based on limits (rate limit and max work limit). The default is unlimited, so even if someone is using their own RegionNormalizer and didn't want limits, they wouldn't get them. But we'd be making the configuration available to all users, which seems like a win with no real downside (given default).

That said, if we decided to keep the shuffling i think that makes sense for SimpleRegionNormalizer to do -- return a shuffled list, rather than have RegionNormalizerWorker shuffle. The shuffling seems ok for the simple normalizer but would probably be unexpected for other implementations.

@charlesconnell charlesconnell Nov 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've moved the truncation into RegionNormalizerWorker


LOG.debug("Computed normalization plans for table {}. Total plans: {}, split plans: {}, "
+ "merge plans: {}", table, plans.size(), splitPlansCount, mergePlansCount);
return plans;
Expand Down Expand Up @@ -464,6 +468,27 @@ private boolean isLargeEnoughForMerge(final NormalizerConfiguration normalizerCo
return getRegionSizeMB(regionInfo) >= normalizerConfiguration.getMergeMinRegionSizeMb(ctx);
}

private List<NormalizationPlan> truncateForSize(List<NormalizationPlan> plans) {
if (
normalizerConfiguration.getCumulativePlansSizeLimitMb() != DEFAULT_CUMULATIVE_SIZE_LIMIT_MB
) {
// If we are going to truncate our list of plans, shuffle the split and merge plans together
// so that the merge plans, which are listed last, are not starved out.
List<NormalizationPlan> maybeTruncatedPlans = new ArrayList<>();
Collections.shuffle(plans);
long cumulativeSizeMb = 0;
for (NormalizationPlan plan : plans) {
cumulativeSizeMb += plan.getPlanSizeMb();
if (cumulativeSizeMb < normalizerConfiguration.getCumulativePlansSizeLimitMb()) {
maybeTruncatedPlans.add(plan);

@bbeaudreault bbeaudreault Nov 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this down a line and instead have this if block return early? Will just cut a little cpu when there are very large plans

@charlesconnell charlesconnell Nov 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
}
return maybeTruncatedPlans;
} else {
return plans;
}
}

private static boolean logTraceReason(final BooleanSupplier predicate, final String fmtWhenTrue,
final Object... args) {
final boolean value = predicate.getAsBoolean();
Expand All @@ -484,6 +509,7 @@ private static final class NormalizerConfiguration {
private final int mergeMinRegionCount;
private final Period mergeMinRegionAge;
private final long mergeMinRegionSizeMb;
private final long cumulativePlansSizeLimitMb;

private NormalizerConfiguration() {
conf = null;
Expand All @@ -492,6 +518,7 @@ private NormalizerConfiguration() {
mergeMinRegionCount = DEFAULT_MERGE_MIN_REGION_COUNT;
mergeMinRegionAge = Period.ofDays(DEFAULT_MERGE_MIN_REGION_AGE_DAYS);
mergeMinRegionSizeMb = DEFAULT_MERGE_MIN_REGION_SIZE_MB;
cumulativePlansSizeLimitMb = DEFAULT_CUMULATIVE_SIZE_LIMIT_MB;
}

private NormalizerConfiguration(final Configuration conf,
Expand All @@ -502,6 +529,8 @@ private NormalizerConfiguration(final Configuration conf,
mergeMinRegionCount = parseMergeMinRegionCount(conf);
mergeMinRegionAge = parseMergeMinRegionAge(conf);
mergeMinRegionSizeMb = parseMergeMinRegionSizeMb(conf);
cumulativePlansSizeLimitMb =
Comment thread
bbeaudreault marked this conversation as resolved.
conf.getLong(CUMULATIVE_SIZE_LIMIT_MB_KEY, DEFAULT_CUMULATIVE_SIZE_LIMIT_MB);
logConfigurationUpdated(SPLIT_ENABLED_KEY, currentConfiguration.isSplitEnabled(),
splitEnabled);
logConfigurationUpdated(MERGE_ENABLED_KEY, currentConfiguration.isMergeEnabled(),
Expand All @@ -512,6 +541,8 @@ private NormalizerConfiguration(final Configuration conf,
currentConfiguration.getMergeMinRegionAge(), mergeMinRegionAge);
logConfigurationUpdated(MERGE_MIN_REGION_SIZE_MB_KEY,
currentConfiguration.getMergeMinRegionSizeMb(), mergeMinRegionSizeMb);
logConfigurationUpdated(CUMULATIVE_SIZE_LIMIT_MB_KEY,
currentConfiguration.getCumulativePlansSizeLimitMb(), cumulativePlansSizeLimitMb);
}

public Configuration getConf() {
Expand Down Expand Up @@ -574,6 +605,10 @@ public long getMergeMinRegionSizeMb(NormalizeContext context) {
}
return mergeMinRegionSizeMb;
}

public long getCumulativePlansSizeLimitMb() {
Comment thread
bbeaudreault marked this conversation as resolved.
Outdated
return cumulativePlansSizeLimitMb;
}
}

/**
Expand Down
5 changes: 5 additions & 0 deletions ...erver/src/main/java/org/apache/hadoop/hbase/master/normalizer/SplitNormalizationPlan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ public NormalizationTarget getSplitTarget() {
return splitTarget;
}

@Override
public long getPlanSizeMb() {
return splitTarget.getRegionSizeMb();
}

@Override
public String toString() {
return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)
Expand Down
35 changes: 35 additions & 0 deletions ...r/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.hadoop.hbase.master.normalizer;

import static java.lang.String.format;
import static org.apache.hadoop.hbase.master.normalizer.SimpleRegionNormalizer.CUMULATIVE_SIZE_LIMIT_MB_KEY;
import static org.apache.hadoop.hbase.master.normalizer.SimpleRegionNormalizer.DEFAULT_MERGE_MIN_REGION_AGE_DAYS;
import static org.apache.hadoop.hbase.master.normalizer.SimpleRegionNormalizer.MERGE_ENABLED_KEY;
import static org.apache.hadoop.hbase.master.normalizer.SimpleRegionNormalizer.MERGE_MIN_REGION_AGE_DAYS_KEY;
Expand All @@ -30,6 +31,7 @@
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.everyItem;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -607,6 +609,39 @@ public void testNormalizerCannotMergeNonAdjacentRegions() {
assertThat(plans, empty());
}

@Test
public void testMergeSizeLimit() {

@bbeaudreault bbeaudreault Nov 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for the shuffle stuff? Just proving that the limit doesn't starve merges like we hope

@charlesconnell charlesconnell Nov 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can write a test but it will be flaky and fail sometimes through bad luck. Collections.shuffle() is not guaranteed to actually change item ordering.

@bbeaudreault bbeaudreault Nov 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided that it was important that the limit be applied approximately fairly to splits and merges. Given that decision, we decided to design our implementation for that goal in mind. Since HBase is an open source project, someone could easily accidentally break this decision down the line. The way to prevent that is be creating tests based on the goals of the code change. Since this was a specific goal, we should probably guard it with a test. The comment helps but isn't really a contract and is often overlooked.

There's a lot of mocking going on in here. It seems like we could think of a way to make a non-flaky test despite the randomness. We aren't testing for a perfect shuffle, we really just want to ensure that at least 1 merge gets into the plan to prove that we aren't starving that.

@charlesconnell charlesconnell Nov 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test

conf.setBoolean(SPLIT_ENABLED_KEY, false);
conf.setLong(CUMULATIVE_SIZE_LIMIT_MB_KEY, 5);
final TableName tableName = name.getTableName();
final List<RegionInfo> regionInfos = createRegionInfos(tableName, 6);
final Map<byte[], Integer> regionSizes = createRegionSizesMap(regionInfos, 1, 1, 1, 1, 1, 1);
setupMocksForNormalizer(regionSizes, regionInfos);
when(tableDescriptor.getNormalizerTargetRegionSize()).thenReturn(2L);

assertTrue(normalizer.isMergeEnabled());
assertFalse(normalizer.isSplitEnabled());
// creates 2 merge plans, even though there are 3 otherwise eligible pairs of regions
assertThat(normalizer.computePlansForTable(tableDescriptor), hasSize(2));
}

@Test
public void testSplitSizeLimit() {
conf.setBoolean(MERGE_ENABLED_KEY, false);
conf.setLong(CUMULATIVE_SIZE_LIMIT_MB_KEY, 10);
final TableName tableName = name.getTableName();
final List<RegionInfo> regionInfos = createRegionInfos(tableName, 4);
final Map<byte[], Integer> regionSizes = createRegionSizesMap(regionInfos, 3, 3, 3, 3);
setupMocksForNormalizer(regionSizes, regionInfos);
when(tableDescriptor.getNormalizerTargetRegionSize()).thenReturn(1L);

assertTrue(normalizer.isSplitEnabled());
assertFalse(normalizer.isMergeEnabled());
// the plan includes only 3 regions, even though the table has 4 otherwise split-eligible
// regions
assertThat(normalizer.computePlansForTable(tableDescriptor), hasSize(3));
}

@SuppressWarnings("MockitoCast")
private void setupMocksForNormalizer(Map<byte[], Integer> regionSizes,
List<RegionInfo> regionInfoList) {
Expand Down