-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Refactor and cleanup treatment of keyspace IDs and KeyRange #12524
Refactor and cleanup treatment of keyspace IDs and KeyRange #12524
Conversation
Signed-off-by: Jeremy Cole <[email protected]>
Signed-off-by: Jeremy Cole <[email protected]>
Signed-off-by: Jeremy Cole <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
If a new flag is being introduced:
If a workflow is added or modified:
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
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.
This is a good change and I think it will resolve the issue related to key range matching.
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.
This is great! Nice work on this, @jeremycole ! ❤️ I only had some minor nits/comments/suggestions. Please let me know what you think.
@jeremycole others have done a detailed review, so I just wanted to chime in say that this is great work! The other proposed changes all seem to be good ones from a code readability / quality / maintainability point of view as well, so there is no objection to proceeding along those lines. You did say in the description that you ran into a bug. Is there an issue for that which can be linked here? If not, do you mind creating one? |
go/vt/key/key.go
Outdated
func Normalize(id []byte) []byte { | ||
if len(id) >= 8 { | ||
return id[:8] |
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.
One breaking change that we should talk about is restricting it is 8-byte length. which the current implementation does not do.
Current implementation support keyspace id to be arbitrary length based on the Vindex.
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.
An example of longer-than-8-byte keyspace IDs can be seen here (link courtesy @harshit-gangal)
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.
Yeah, I (also) mistakenly thought they were always 64 bits, but that's only the length of the default hash
vindex based keyspace IDs. Some are shorter and others longer. You can see examples of how many hex digits are produced for each vindex type by using the vindex query functions: https://vitess.io/docs/17.0/reference/features/vindexes/#query-vindex-functions
I was also pointed this blog post that I don't remember previously reading: https://vitess.io/blog/2017-09-18-custom-sharding-with-vitess/
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.
I see what you mean here. I am not sure that's a real case that anyone is using but I am quite okay with not changing the semantics here. We can pivot the Normalize
method to, instead of padding out to a fixed length (which there isn't any value that would satisfy the requirements without defining a maximum length) to instead remove all trailing zeroes consistently.
I made that change locally and it works fine and all tests pass the same except for TestNormalize
(since it's explicitly checking the return values of Normalize
, so it would be expected). I'll push another commit for this.
Signed-off-by: Hormoz Kheradmand <[email protected]>
Thanks for the prompting, I filed #12535 with the actual bug we hit that caused us to go down this rabbit hole. :) |
Signed-off-by: Jeremy Cole <[email protected]>
Signed-off-by: Jeremy Cole <[email protected]>
Signed-off-by: Jeremy Cole <[email protected]>
Signed-off-by: Jeremy Cole <[email protected]>
Signed-off-by: Jeremy Cole <[email protected]>
… tests Signed-off-by: Jeremy Cole <[email protected]>
👋 I think I have addressed all review feedback with the last commits pushed. Sorry for adding everyone as reviewers, every time I touch a new file it seems we get a new reviewer added automatically. 😄 I broke up the additional work into several separate commits so that those changes can more easily be reviewed in isolation if you so desire. Let me know anything else you need! (And we'll see how the full CI runs go.) |
Looks like you broke something, both |
The failure in I haven't quite gotten the |
Looks like the local example failure is a flake as well. I see it failing on a PR that touches no code here #12566 |
@jeremycole I re-ran the 3 failing tests. I've also invited you to the org, so that in future you will have the ability to do this too. Invite goes to your primary GH email. |
@mattlord does this address your comments and concerns? |
👋 We're still holding on this but using it in our internal branch. Are there any remaining concerns about it? |
Hey! Sorry for the delay. I'll add this review to my ToDo for tomorrow. |
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.
Nice work on this, @jeremycole ! The quality of the code, comments, PR description, issue etc are all very good. ❤️
Description
Fixes #12535
Initially we encountered a bug where unevenly split shards with a set of key ranges as (...
000280-000300
,0003-
) did not properly initialize shard000280-000300
due to believing it overlapped with0003-
. (This was due to a comparison where000300
should have been equal to0003
but was not.)In the course of fixing this bug, I decided to refactor and cleanup the treatment of keyspace IDs and
topodatapb.KeyRange
values overall. The treatment of keyspace IDs was very inconsistent resulting in many comparisons not properly handling theKeyRange
-specific edge cases (mainly due to logic usingbytes.Compare
scattered across the codebase):Start
andEnd
fields comparing empty/zero values as minimum-key and maximum-key, respectively.Start
orEnd
fields containingnil
and zero-length[]byte
.80
vs.8000
. (Attempts were made previously to resolve this withaddPadding
which proved incomplete; my initial fix which we shipped/tested internally added calls toaddPadding
to everywhere keyspace IDs were being passed, which was effective but very ugly and poorly factored.)In addition I've added a number of tests and improved test coverage of much of the existing code, as well as clarifying existing comments and adding new ones. All of the existing tests continue to pass, with one exception which appears to be due to a bug in the original code which was then codified in the corresponding test:
Side quest: a bug fix for
TestKeyRangeContiguous
TestKeyRangeContiguous
tested"-"
and"-40"
and expected a result oftrue
, but those key ranges are most definitely not contiguous. The corresponding code had the following set of logic:That's attempting to handle
nil
values (although inconsistently) but treatingnil
as a missing value (inconsistently), whereas the rest of the code treatsnil
as full-range. I believe this should as well. I've replaced it with new logic, which I believe to be more correct:If there is some reason this should treat
nil
differently, let me know and I'll be happy to fix that and document it. However, there is only one call site and it doesn't read to me like it requires any special/different handling, so I believe this was just a misunderstanding.Future work
There's still a fair bit of work to be done to clean this up, I believe at least the following would be reasonable:
type KeyspaceID
or similar for keyspace IDs and replace usage of[]byte
to the extent possible.type Uint64Key
(which is only used in tests) in favor of the newtype
.type KeyRange
, re-organize all the various comparison functions, and consistently handle both parsing and formatting KeyRanges in a single set of functions.EvenShardsKeyRange
andGenerateShardRanges
which behave very questionably at best and are rarely used.I did not attempt to do any of these here to keep the PR size reasonable, but if there is some level of agreement (or at least a lack of disagreement) I'd be happy to undertake them.
Related Issue(s)
Checklist
Deployment Notes
There should not be any user-facing impact of this change, as it's purely backend and bug fixing existing functionality which was perhaps never used before.