REP-5328 Compare shard keys as part of metadata verification. #63
Conversation
tdq45gj
left a comment
tdq45gj
left a comment
There was a problem hiding this comment.
This looks good in general. I wonder if ServerThinksTheseMatch should be a method on verifier and always use the meta client.
| // pipeline (`a` and `b`, respectively) before they're compared. | ||
| func ServerThinksTheseMatch( | ||
| ctx context.Context, | ||
| client *mongo.Client, |
There was a problem hiding this comment.
It looks like we want to restrict this function to only run on the meta cluster. Can we make it a method of the verifier and make sure that the query only runs on the meta cluster?
There was a problem hiding this comment.
I actually think it’s better as a static function so that there’s better separation of concerns. The more things we “hang” on Verifier, the less modular it all is.
There was a problem hiding this comment.
Can we add a comment that to the function that explains it expects the client to only be the meta client for most of the cases?
internal/util/sharding.go
Outdated
| Database(configDBName). | ||
| Collection(collsCollName) | ||
|
|
||
| rawResult, err := configCollectionsColl.FindOne(ctx, bson.D{{"_id", namespace}}).Raw() |
There was a problem hiding this comment.
Can we define a struct to consume the sharding config doc? I think it will make the code more straightforward.
| cursor, err := configCollectionsColl.Find(ctx, bson.D{{"uuid", uuid}}) | ||
| func (verifier *Verifier) getShardKeyFields( | ||
| ctx context.Context, | ||
| namespaceAndUUID *uuidutil.NamespaceAndUUID, |
There was a problem hiding this comment.
Will this parameter ever be nil?
There was a problem hiding this comment.
Likely not, but this is just reformatted code.
| // ServerThinksTheseMatch runs an aggregation on the server that determines | ||
| // whether the server thinks a & b are equal. This allows you, e.g., to |
There was a problem hiding this comment.
The comment is not very accurate. It has a shortcut that compares the values in memory first.
There was a problem hiding this comment.
That’s an implementation, though. It seems trivially true that two binary-equivalent BSON values are ones that the server would also consider equivalent.
2 participants
This entails a fair bit of moving & renaming code, but the actual logic added is fairly simple:
util.Small fixes:
getIndexesMap()now correctly checks for error before checking for an index name.