Uh oh!
There was an error while loading. Please reload this page.
HBASE-27496 Optionally limit the cumulative size of normalization plans produced by SimpleRegionNormalizer - #4888
Merged
bbeaudreault merged 6 commits intoNov 22, 2022
Conversation
…by SimpleRegionNormalizer This commit adds two new settings: - hbase.normalizer.merge.plans_size_limit.mb - hbase.normalizer.split.plans_size_limit.mb These settings limit the number of plans produced by a single run of SimpleRegionNormalizer, by forcing it to stop producing new plans once the cumulative region size limits are exceeded.
Apache-HBase
commented
Nov 18, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 18, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 18, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 19, 2022
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
You might need to run mvn spotless:apply if you haven't already. There were also some checkstyle warnings before that need to be addressed if they're still applicable.
Otherwise just 2 small requests. Thanks!
| } | ||
| @Test | ||
| public void testMergeSizeLimit() { |
Contributor
There was a problem hiding this comment.
Can you add a test for the shuffle stuff? Just proving that the limit doesn't starve merges like we hope
ContributorAuthor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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.
| for (NormalizationPlan plan : plans) { | ||
| cumulativeSizeMb += plan.getPlanSizeMb(); | ||
| if (cumulativeSizeMb < normalizerConfiguration.getCumulativePlansSizeLimitMb()) { | ||
| maybeTruncatedPlans.add(plan); |
Contributor
There was a problem hiding this comment.
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
| plans.addAll(mergePlans); | ||
| } | ||
| plans = truncateForSize(plans); |
Contributor
There was a problem hiding this comment.
My only other thought is whether it makes sense to do the truncating in the NormalizationWorker, like how it currently handles rate limiting
ContributorAuthor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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.
ContributorAuthor
There was a problem hiding this comment.
Sure, I've moved the truncation into RegionNormalizerWorker
Apache-HBase
commented
Nov 19, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 19, 2022
💔 -1 overall
This message was automatically generated. |
bbeaudreault
self-requested a review
November 21, 2022 13:15
charlesconnellforce-pushed
the
HBASE-27496/normalizer-plan-limits
branch
from
November 21, 2022 17:35
21240f6 to
7c2025bCompareApache-HBase
commented
Nov 21, 2022
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
requested changes
Nov 21, 2022
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
cool, a few more small comments and then i think we're good to go. just logging/config fixes
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Nov 21, 2022
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
approved these changes
Nov 21, 2022
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
looks good, thanks! i'll merge once the 3 pre-commits all come back green
Apache-HBase
commented
Nov 22, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 22, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 22, 2022
💔 -1 overall
This message was automatically generated. |
bbeaudreault
commented
Nov 22, 2022
Contributor
The test failures look unrelated but unfortunately we do have to fix the checkstyle issue. Running spotless:apply may fix it. |
Apache-HBase
commented
Nov 22, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 22, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 22, 2022
🎊 +1 overall
This message was automatically generated. |
charlesconnell added a commit
to HubSpot/hbase
that referenced
this pull request
Nov 22, 2022
…alizer (apache#4888) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault pushed a commit
that referenced
this pull request
Nov 22, 2022
…alizer (#4888) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault pushed a commit
that referenced
this pull request
Nov 22, 2022
…alizer (#4888) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault pushed a commit
to HubSpot/hbase
that referenced
this pull request
Nov 22, 2022
…ecuted in the Normalizer (apache#4888) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the new setting
hbase.normalizer.plans_size_limit.mb. This setting limit the number of plans processed by RegionNormalizerWorker, by forcing it to stop processing plans once the cumulative region size limits are exceeded.