-
Notifications
You must be signed in to change notification settings - Fork 34
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.
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
DBZ-8163 Add inherit epoch feature #208
Conversation
@jpechane can you review this when you get the chance? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, left a few small comments.
} | ||
|
||
public boolean overlaps(Shard shard) { | ||
return this.lowerBound.compareTo(shard.upperBound) < 0 && this.upperBound.compareTo(shard.lowerBound) > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the situation when the two ranges meet on the boundary ( == 0 case)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vitess ranges are have inclusive lower bounds and exclusive upper bounds so this should be correct (docs). We can think about each part:
this.lowerBound.compareTo(shard.upperBound) == 0
the lower bound is the same as the upper bound we are comparing to. Since the upper bound is exclusive, this interval does not overlap.this.upperBound.compareTo(shard.lowerBound) == 0
the upper bound is the same as the lower bound we are comparing to. Since the upper bound is exclusive, these two intervals do not overlap.
I added a test to demonstrate this case.
// A string lexicographically less than all other strings | ||
public static final String NEGATIVE_INFINITY = ""; | ||
// A string lexicographically greater than all other strings | ||
public static final String POSITIVE_INFINITY = "\uFFFF"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 4F long/big enough, do we need 8F? or have an assert if we sees the upper bound is more than 4 characters long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this \uFFFF
is actually a single special char that is greater than all other chars. I used the Character package to make this clear (it is the MAX_VALUE). And it turns out there is MIN_VALUE which is another special char (more clear than empty string). I updated the comments to describe this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, the change looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! I left just one proposal related to option name. Other than that this is good to go.
@@ -301,6 +302,15 @@ public static BigIntUnsignedHandlingMode parse(String value, String defaultValue | |||
.withDescription("Control StopOnReshard VStream flag." | |||
+ " If set true, the old VStream will be stopped after a reshard operation."); | |||
|
|||
public static final Field INHERIT_EPOCH = Field.create(VITESS_CONFIG_GROUP_PREFIX + "inherit_epoch") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final Field INHERIT_EPOCH = Field.create(VITESS_CONFIG_GROUP_PREFIX + "inherit_epoch") | |
public static final Field INHERIT_EPOCH = Field.create(VITESS_CONFIG_GROUP_PREFIX + "inherit.epoch") |
@twthorn Applied, thanks! |
Add a config feature for inherit epoch. When enabled, the parent shards will be deduced from the hex ranges and we will take the max of the parent shards + 1 to get a newer epoch value. Refactor some things to allow us to read in this config. Also refactor the epoch logic so that our shard epoch map does not drift from the vgtid we receive.
I am hoping to expand our integration test suite in a separate PR to test reshard operations. Since this PR is already substantial putting this out now and will have a separate one for adding reshards to itests.