Skip to content

Conversation

@FGasper
Copy link
Collaborator

@FGasper FGasper commented Nov 22, 2024

Previously migration-verifier only checked the parts of index specs that go in a mongo.IndexSpecification. That struct is incomplete, though; it doesn’t, for example, include partial index filters. Thus, migration-verifier didn’t compare those parts of an index specification.

This changeset alters the index-checking logic to check full index specifications. Note that this comparison is rather “coarse” and may flag some specifications as mismatching when they actually produce the same index. For example, {foo: 1} and {foo: 2} actually produce the same index, but migration-verifier will flag them as mismatches. Eventually we hope to fix that, but for now false positives are safer than incomplete checks.

This also replaces use of mongo.CollectionSpecification with a struct that allows detection of unrecognized fields. Thus we’ll avoid a similar problem for collection specifications.

This also copies over mongosync’s Option implementation.

@FGasper FGasper requested a review from tdq45gj November 22, 2024 19:30
@FGasper FGasper marked this pull request as ready for review November 22, 2024 19:30
Copy link
Collaborator

@tdq45gj tdq45gj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Field: "ExpireAfterSeconds",
Details: Mismatch + fmt.Sprintf(" : src: %s, dst: %s", nilableToString(srcSpec.ExpireAfterSeconds), nilableToString(dstSpec.ExpireAfterSeconds))})
// Next check to see if the only differences are type differences.
// If we didn’t support pre-v5 servers we could use a $documents aggregation,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can reasonably limit the metadata cluster to post-v5 versions, but probably doesn't worth to do only for $document.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh, right! I’d overlooked that this is the metadata cluster. Totally worth it, then, IMO.

@FGasper FGasper merged commit c0c8966 into mongodb-labs:main Nov 25, 2024
33 checks passed
@FGasper FGasper deleted the REP-5274-index-comparison branch November 25, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants