Skip to content

GODRIVER-1469 ensure gridfs index checking supports indexes created i… #341

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

Merged
merged 7 commits into from
Mar 27, 2020

Conversation

iwysiu
Copy link
Contributor

@iwysiu iwysiu commented Mar 19, 2020

…n the shell

@iwysiu iwysiu requested review from jyemin and divjotarora March 19, 2020 20:08
Copy link
Contributor

@divjotarora divjotarora left a comment

Choose a reason for hiding this comment

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

As a starting point, I'm finding the code for createIndexIfNotExists hard to read because the variable names get tricky when we do multiple BSON transformations. I think it would help if we added a new function called compareIndexDocs(expected, actual bsoncore.Document) that did both a bytes.Equal check as well as a semantic element-wise check. I want to get your opinion on this before continuing the review.

@iwysiu iwysiu requested a review from divjotarora March 20, 2020 15:05
@iwysiu iwysiu requested a review from divjotarora March 20, 2020 18:26
Copy link
Contributor

@divjotarora divjotarora left a comment

Choose a reason for hiding this comment

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

One nit, but otherwise LGTM. Have you verified that the test fails in our existing implementation?

Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of tests that I would like to see added.

@iwysiu iwysiu requested a review from jyemin March 27, 2020 14:34
@divjotarora
Copy link
Contributor

@iwysiu The "config file was invalid" error from Evergreen can be fixed by rebasing on master and force-pushing. The Evergreen team invalidated the cron syntax we were using because it was subtly incorrect.

Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

LGTM!

@iwysiu iwysiu merged commit d947d92 into mongodb:master Mar 27, 2020
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.

3 participants