REP-6465 Fix handling of dotted shard keys #127
Conversation
FGasper
changed the title
There was a problem hiding this comment.
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.
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.
Typically this is called in a defer right after we open successfully.
There was a problem hiding this comment.
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 <[email protected]>
…gration-verifier into REP-6465-fix-dotted-shard-key
This amends PR mongodb-labs#127 so that, if a document lacks a shard key field, the generated document key will include that field with a null value. This matches the server’s internal logic and seems safer besides.
This fixes intermittent failures in PR mongodb-labs#127’s new dotted-key sharding test.
This amends PR mongodb-labs#127 so that, if a document lacks a shard key field, the generated document key will include that field with a null value. This matches the server’s internal logic and seems safer besides.
This fixes intermittent failures in PR mongodb-labs#127’s new dotted-key sharding test.
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., thebarfield 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
documentKeyinaccurately reports the values used to route the document.