HBASE-25766 Introduce RegionSplitRestriction that restricts the … #3150
Conversation
…pattern of the split point
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I don't think the unit test failures are related to this change. When I did run the failed tests locally, they were successful. @Apache9 Can you please review it? Thanks. |
There was a problem hiding this comment.
In general LGTM.
But on the naming, I'm not sure if RegionSplitPointRestriction is a good name...
Ping @saintstack. Do you have any suggestions on the naming?
Thanks.
| TableDescriptor tableDescriptor = env.getMasterServices().getTableDescriptors() | ||
| .get(getTableName()); | ||
| Configuration conf = env.getMasterConfiguration(); | ||
| if (hasBestSplitRow()) { |
There was a problem hiding this comment.
We do not have this logic in the past? What is changed so now we need to apply this restriction in SplitTableRegionProcedure? IIRC the logic is done at region server side?
There was a problem hiding this comment.
Yes, we didn't have this logic in the past. I think we can apply the restriction to a user-specified split point because without this logic, we can easily break the restriction by splitting with specifying a split point. And the user-specified split point is passed to the Master side, we need to do it on the master side.
What do you think? @Apache9
There was a problem hiding this comment.
Finally we should get the actual split point back from region server? No? Then this should be a bug for the current code base?
There was a problem hiding this comment.
Finally we should get the actual split point back from region server? No?
No, I don't think so.
Let's see we have a table that has a key prefix restriction where the prefix length is 2 bytes.
When a user runs split command with specifying a split point abc in the hbase shell, this will break the key prefix restriction if we split the region by abc. So I think we can apply the restriction to the user-specified split point, and the restriction-applied split point will be ab, which won't break the restriction.
There was a problem hiding this comment.
@Apache9 What about this? Thanks.
There was a problem hiding this comment.
Good point. So user may be 'surprised' if we do not split where they want? Will there be a message saying so anywhere that their choice has been over-ruled by the restriction? Or will it be obvious that the 'restriction' over-ruled?
I'm good w/ the restriction over-ruling the user as long as there a log to this effect (add the 'behavior change' to the existing nice release note @brfrn169 )
There was a problem hiding this comment.
I think we can add a WARN message in the Master log when the user-specified split point is over-ruled by the restriction. I will do that. And I will add the 'behavior change' to the release note. Thanks.
There was a problem hiding this comment.
So here we will only use SplitRestriction to fix the split row? Then what if users uses the deprecated KeyPrefixSplitPolicy? We will not fix the split row if it breaks the rule?
There was a problem hiding this comment.
Yes, that's the behavior of KeyPrefixSplitPolicy, and we will not fix the split row even if it breaks the rule. And maybe it's not easy to fix it because RegionSplitPolicy doesn't have any method to restrict/convert a user-specified split point. It has only byte[] getSplitPoint() that gets an appropriate split point calculated based on its policy.
There was a problem hiding this comment.
OK, so in fact we are not changing the behavior? If you use the old KeyPrefixSplitPolicy, there is nothing changed. If you use the new SplitRestriction, then you will find out that you are not allowed to break the restriction when proposing a split point. Could mention this in the release note.
...src/main/java/org/apache/hadoop/hbase/regionserver/KeyPrefixRegionSplitPointRestriction.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/apache/hadoop/hbase/regionserver/NoneRegionSplitPointRestriction.java
Outdated
Show resolved
Hide resolved
...e-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionSplitPointRestriction.java
Outdated
Show resolved
Hide resolved
|
@saintstack Can you please take a look at this? |
There was a problem hiding this comment.
A few notes below @brfrn169 ... Pardon my taking so long to respond
| * | ||
| * This ensures that a region is not split "inside" a prefix of a row key. | ||
| * I.e. rows can be co-located in a region by their prefix. | ||
| */ |
There was a problem hiding this comment.
Examples help (You have some elsewhere... If you are making a new PR, add one here too?)
There was a problem hiding this comment.
I have put some examples in the Release Note in the Jira:
https://issues.apache.org/jira/browse/HBASE-25766
Can you please check it? And if it's not enough, please let me know. Thanks.
There was a problem hiding this comment.
Was thinking here in code. If making a new PR, can add it.. else it fine.
There was a problem hiding this comment.
Sure. Maybe we can add the example in the JavaDoc in the RegionSplitRestriction class. I will do that. Thanks.
...rver/src/main/java/org/apache/hadoop/hbase/regionserver/NoneRegionSplitPointRestriction.java
Outdated
Show resolved
Hide resolved
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * A split point restriction that restricts the pattern of the split point. |
There was a problem hiding this comment.
I think say more about Restriction ... and how they work here and why they can't be done as split policy.
There was a problem hiding this comment.
... on how a *RegionSplitRestriction differs from a *RegionSplitPolicy?
There was a problem hiding this comment.
Do we need the 'Point' in the class names? Isn't RegionSplit enough? Do we need to say RegionSplitPoint? Saying RegionSplit rather than RegionSplitPoint ties these classes to RegionSplitPolicy... which seems good but maybe you don't want that?
There was a problem hiding this comment.
The split point restriction is what I proposed in HBASE-25706. As here we have two dimentions, one is when do we need to split, such as IncreasingToUpperBound, or ConstantSize, another is how to split, such as KeyPrefix, DelimitedKeyPrefix.
There was a problem hiding this comment.
Thank you for explaining it! @Apache9
And thank you for reviewing it! @saintstack
Yes, it seems like we have two dimensions in the current RegionSplitPolicy. And this PR will separate one of the dimensions from RegionSplitPolicy into RegionSplitPointRestriction.
And thank you for the excellent suggestion to rename RegionSplitPointRestriction to RegionSplitRestriction. I will rename it that way. Thanks.
This comment has been minimized.
This comment has been minimized.
brfrn169
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@saintstack @Apache9 I added a WARN message when the user-specified split point is over-ruled by the restriction as follows: And I also added JavaDoc for RegionSplitRestriction as follows: Please let me know if you have any other things we should to do here. |
4 participants
…pattern of the split point