HBASE-27667 Normalizer can skip picking presplit regions while prepar… #5170
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
chrajeshbabu
requested review from
bbeaudreault,
ndimiduk and
Apache9
and removed request for
bbeaudreault
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Apr 12, 2023
| @@ -246,23 +251,35 @@ public List<NormalizationPlan> computePlansForTable(final TableDescriptor tableD | |||
|
|
|||
| /** Returns size of region in MB and if region is not found than -1 */ | |||
| private long getRegionSizeMB(RegionInfo hri) { | |||
| RegionMetrics regionLoad = getRegionMetrics(hri); | |||
| if (regionLoad == null) return -1; | |||
There was a problem hiding this comment.
This will generate a checkstyle warning? Please use {} to wrap the block.
|
|
||
| private long getStoreFileCount(RegionInfo hri) { | ||
| RegionMetrics regionLoad = getRegionMetrics(hri); | ||
| if (regionLoad == null) return 0; |
| @@ -474,6 +494,11 @@ private boolean isLargeEnoughForMerge(final NormalizerConfiguration normalizerCo | |||
| return getRegionSizeMB(regionInfo) >= normalizerConfiguration.getMergeMinRegionSizeMb(ctx); | |||
| } | |||
|
|
|||
| private boolean isPresplitEmptyRegion(final NormalizerConfiguration normalizerConfiguration, | |||
| final NormalizeContext ctx, final RegionInfo regionInfo) { | |||
| return getStoreFileCount(regionInfo) == 0 && | |||
There was a problem hiding this comment.
Why getStoreFileCount == 0 means this is a pre split region?
There was a problem hiding this comment.
Why getStoreFileCount == 0 means this is a pre split region?
Thanks for review @Apache9. When the region empty means there are three scenarios 1) pre split region where there are no storefiles and region size is 0(in the normalization we are not considering memstore data or live updates so considering store files count 0 as one of the option to detect presplit region) 2) TTL expired for region then major compaction cleanup but create zero size store files where count is more than 0 but region size is 0 3) All the region data is deleted in this case also major compaction creates empty store files but region size is 0.
Will clear other checkstyle and spotless checks.
There was a problem hiding this comment.
I think that this single empty store file behaviour is a coincidence, not something that we want to design a functionality around.
Original implementation of normaliser had a long discussion about how to handle pre-split regions. As I recall, the conclusion was to use the minimum age setting as a means for an operator to preserve an empty, pre-split table. I don't particularly like this approach either, but at least it means that any forgotten empty regions will eventually converge on a more correct state.
If you want to pursue some other means, I prefer it involve explicit metadata instead of more implicit assumptions (especially about implementation details). Sorry @chrajeshbabu , but I strongly disapprove of this approach.
There was a problem hiding this comment.
@ndimiduk agree with you. Will close this PR as of now as there is no other means to detect the presplit regions from meta data and reopen in case of any best option.
ndimiduk
requested changes
Apr 17, 2023
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.
…ing merge plans