Skip to content
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

Fix the bug where dropIndex command is not replying all necessary fie… #175

Conversation

dongxinEric
Copy link
Contributor

…lds in error cases. Audited all places where ok is 0 to make sure $err and errmsg are both returned

…lds in error cases. Audited all places where ok is 0 to make sure $err and errmsg are both returned
@dongxinEric dongxinEric requested a review from apkar April 24, 2019 16:41
@dongxinEric dongxinEric self-assigned this Apr 24, 2019
Copy link
Contributor

@apkar apkar left a comment

Choose a reason for hiding this comment

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

LGTM. There is one question on the code this patch is touching. This patch is not making anything worse, though.

@@ -608,7 +608,11 @@ ACTOR static Future<Reference<ExtMsgReply>> doDropIndexesActor(Reference<ExtConn
reply->addDocument(BSON("nIndexesWas" << dropped + 1 << "ok" << 1.0));
return reply;
} else {
reply->addDocument(BSON("ok" << 0.0));
// clang-format off
reply->addDocument(BSON("ok" << 0.0 <<
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you didn't change this. Can you make sure if ok value needs to be float or int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are fine. I believe in most main stream languages, user can compare double, float, int and long with each other without having to explicitly cast the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And our code base does have mixed usage of double and int, and sometime long. We'd better clean that up too someday.

Copy link
Contributor

@apkar apkar Apr 24, 2019

Choose a reason for hiding this comment

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

Can you please raise an issue for that? Also, OP_REPLY response flag need to be set apparently for $err to work. We do set for OP_GETMORE but nowhere else. You might want to mention that in the issue.

https://docs.mongodb.com/v3.0/reference/mongodb-wire-protocol/#wire-op-reply

@dongxinEric dongxinEric merged commit 955f947 into FoundationDB:master Apr 24, 2019
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