Skip to content

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.

Sign up for GitHub

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 1 commit into from
Feb 7, 2019
Merged

RUBY-1420 Document session options #1239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions lib/mongo/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,10 +589,11 @@ def list_mongo_databases(filter = {}, opts = {})
# @example Start a session.
# client.start_session(causal_consistency: true)
#
# @param [ Hash ] options The session options.
# @param [ Hash ] options The session options. Accepts the options
# that Session#initialize accepts.
#
# @note A Session cannot be used by multiple threads at once; session objects are not
# thread-safe.
# @note A Session cannot be used by multiple threads at once; session
# objects are not thread-safe.
#
# @return [ Session ] The session.
#
Expand Down
1 change: 1 addition & 0 deletions lib/mongo/database.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ def collections
# @param [ Hash ] opts The command options.
#
# @option opts :read [ Hash ] The read preference for this command.
# @option opts :session [ Session ] The session to use for this command.
#
# @return [ Hash ] The result of the command execution.
def command(operation, opts = {})
Expand Down
7 changes: 7 additions & 0 deletions lib/mongo/session.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,13 @@ class Session
# @param [ Client ] client The client through which this session is created.
# @param [ Hash ] options The options for this session.
#
# @option options [ true|false ] :causal_consistency Whether to enable
# causal consistency for this session.
# @option options [ Hash ] :default_transaction_options Options to pass
# to start_transaction by default, can contain any of the options that
# start_transaction accepts.
# @option options [ true|false ] :implicit For internal driver use only -
# specifies whether the session is implicit.
# @option options [ Hash ] :read_preference The read preference options hash,
# with the following optional keys:
# - *:mode* -- the read preference as a string or symbol; valid values are
Expand Down
4 changes: 1 addition & 3 deletions spec/support/transactions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,7 @@ def setup_test
coll = support_client[@spec.collection_name]
coll.database.drop
coll.with(write: { w: :majority }).drop
support_client.command(
{ create: @spec.collection_name },
{ write_concern: { w: :majority } })
support_client.command(create: @spec.collection_name)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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


coll.with(write: { w: :majority }).insert_many(@data) unless @data.empty?
admin_support_client.command(@fail_point) if @fail_point
Expand Down