-
Notifications
You must be signed in to change notification settings - Fork 15
REP-6465 Fix handling of dotted shard keys #127
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
base: main
Are you sure you want to change the base?
Conversation
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 mean, this seems fine? I'm deeply suspicious of the aggregation pipeline because I do not feel at all qualified to review that, so you might also get a review from someone that does. The tests seem to pass though, but you might want some more cases for weird things (notably, arrays where you might get depth-traversal errors).
internal/verifier/compare.go
Outdated
parts := strings.Split(field, ".") | ||
val, err := doc.LookupErr(parts...) | ||
|
||
if errors.Is(err, bsoncore.ErrElementNotFound) { |
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.
You probably also want to handle bsoncore.ErrInvalidDepthTraversalError
here, which is the kind of error you get if you have a shard key like a.b
and the a
field in the target document is an array.
internal/verifier/dockey_agg_test.go
Outdated
|
||
require.NoError(changes.Err(), "change stream should not fail") | ||
|
||
changes.Close(ctx) |
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.
Typically this is called in a defer
right after we open successfully.
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 didn’t have it that way because it was in a loop, but I’ve rewritten it now to be a subtest, which has its own callback and so can use defer.
Co-authored-by: Michael McClimon <michael@mcclimon.org>
…gration-verifier into REP-6465-fix-dotted-shard-key
This fixes Migration Verifier’s logic to assemble a document key given a full document and field names when any of those fields has a dot.
When the server routes a document whose collection is sharded on, e.g.,
{foo.bar: 1}
, it does so based on the value of /foo/bar (i.e., thebar
field of the top-level embedded document namedfoo
) in the document. Migration Verifier instead assembled its document key from the value at /foo.bar (i.e., a top-level field namedfoo.bar
).This fixes the bug. Notably, it also avoids the problem, documented in SERVER-109340, where the server’s change stream’s
documentKey
inaccurately reports the values used to route the document.