HBASE-25534 Honor TableDescriptor settings earlier in normalization #2917
Conversation
This file contains 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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
reviewed
Feb 3, 2021
| LOG.warn("TableDescriptor for {} unavailable, table-level target region count and size" | ||
| + " configurations cannot be considered.", table, e); | ||
| } | ||
| long targetRegionCount = tableDescriptor.getNormalizerTargetRegionCount(); |
There was a problem hiding this comment.
nit: no need to promote this to a long.
| @@ -316,10 +314,10 @@ private double getAverageRegionSizeMb(final List<RegionInfo> tableRegions) { | |||
| */ | |||
| private boolean skipForMerge( | |||
| final NormalizerConfiguration normalizerConfiguration, | |||
| final RegionStates regionStates, | |||
| final NormalizeContext ctx, | |||
| @@ -468,12 +467,13 @@ private static boolean isOldEnoughForMerge( | |||
| */ | |||
| private boolean isLargeEnoughForMerge( | |||
| } | ||
|
|
||
| private static boolean logTraceReason(final BooleanSupplier predicate, final String fmtWhenTrue, | ||
| private boolean logTraceReason(final BooleanSupplier predicate, final String fmtWhenTrue, |
There was a problem hiding this comment.
why make this an instance method?
There was a problem hiding this comment.
I find skipForSplit, logTraceReason, isOldEnoughForMerge is static, but it does not need static. I am not sure what is the purpose of setting those to static.
There was a problem hiding this comment.
As a general practice, when a method can be static, I prefer to make it so. If a method has no need for this or private state, why expose it? I find static methods are easier to reason about because you know there's no hidden effects of object member variables manipulated elsewhere. I suspect they're simpler for the JIT to optimize for the same reasons, but I don't have evidence of this.
There was a problem hiding this comment.
Thanks for the wonderful answers, which benefited me a lot.
| @@ -592,5 +621,14 @@ public RegionStates getRegionStates() { | |||
| public double getAverageRegionSizeMb() { | |||
| return averageRegionSizeMb; | |||
| } | |||
|
|
|||
| public <T> T getOrDefault(String key, Function<String, T> function, T defaultValue) { | |||
There was a problem hiding this comment.
nice.
Maybe this method returns an Object/null and the caller handles the null-check. I think this approach would be better than using an out-of-range value to indicate the lack of configuration.
| } | ||
|
|
||
| @Test | ||
| public void testHonorsSplitEnabledInTD() { |
There was a problem hiding this comment.
These tests *EnabledInTD are good for confirming that the split/merge will not happen when disabled in table descriptor. Please also add coverage of when split/merge is disabled in configuration but enabled in table descriptor. I believe this should be supported, given the new implementations of proceedWith{Split,Merge}Planning.
|
Thanks @ndimiduk. Not sure about the static keyword. The rest is done. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
changed the title
ndimiduk
pushed a commit
to ndimiduk/hbase
that referenced
this pull request
Feb 10, 2021
…pache#2917) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
ndimiduk
pushed a commit
that referenced
this pull request
Feb 10, 2021
…2917) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
ddupg
pushed a commit
to ddupg/hbase
that referenced
this pull request
Feb 19, 2021
…pache#2917) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Feb 24, 2021
…pache#2917) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
ZhaoBQ
added a commit
to ZhaoBQ/hbase
that referenced
this pull request
May 25, 2021
…lization (apache#2917)" to branch-2
ndimiduk
pushed a commit
that referenced
this pull request
May 25, 2021
…2917) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Table can be enabled and disabled merge/split in TableDescriptor, we should judge before calculating the plan.
Part of the configuration can be set by table level. For example, hbase.normalizer.min.region.count can be set by "alter ‘table’, CONFIGURATION=>{'hbase.normalizer.min.region.count' => '5'}". If the table is not set, then use the global configuration which is set in hbase-site.xml.