-
Notifications
You must be signed in to change notification settings - Fork 532
RUBY-1420 Document session options #1239
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
Conversation
support_client.command( | ||
{ create: @spec.collection_name }, | ||
{ write_concern: { w: :majority } }) | ||
support_client.command(create: @spec.collection_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec test runner description says that this should be run with write concern majority; is there a reason this was changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to my reading of the code write concern ended up getting passed to sessions where it was ignored. Database#command (which can be seen in this diff) does not document write concern as an accepted option. Therefore as far as I can tell the write concern here doesn't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting; it looks like the read/write concern spec says that a write concern needs to be directly added to the command document. Can we do this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since repairing transaction spec runner does not have much to do with documenting session options, how about doing that in a follow-up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, why was it changed in this PR in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR documents session options. As part of that work I reviewed the callers to see which options they passed. As a result of that audit this change was made where existing code passed an option that had no effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a little strange not to not make the one line fix here when we're already changing the same method call, but okay
support_client.command( | ||
{ create: @spec.collection_name }, | ||
{ write_concern: { w: :majority } }) | ||
support_client.command(create: @spec.collection_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a little strange not to not make the one line fix here when we're already changing the same method call, but okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, meant to approve
No description provided.