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

DOCSP-37046: csot in v2 #375

Merged
merged 7 commits into from
Aug 12, 2024
Merged

Conversation

rustagir
Copy link
Collaborator

@rustagir rustagir commented Aug 6, 2024

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-37046
Staging:

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?
  • Are the facets and meta keywords accurate?

Copy link
Collaborator

@jordan-smith721 jordan-smith721 left a comment

Choose a reason for hiding this comment

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

Nice work! Just a few comments and clarifications

Comment on lines 24 to 25
options. You can pass the connection options as parameters of the connection
URI to specify the behavior of the client.
Copy link
Collaborator

Choose a reason for hiding this comment

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

S:

Suggested change
options. You can pass the connection options as parameters of the connection
URI to specify the behavior of the client.
options. You can pass the connection options as parameters in the connection
URI to specify the behavior of the client.

Comment on lines 53 to 54
- Specifies the number of milliseconds to wait before timeout on a TCP
connection.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be out of scope of this PR, but what is a TCP connection? Should readers know this already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rewrote the desc to leave the acronym out (simplify)

Comment on lines 125 to 126
You can set a single ``Timeout`` option on your ``Client`` instance to
specify the amount of time that a single operation can take to execute.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You can set a single ``Timeout`` option on your ``Client`` instance to
specify the amount of time that a single operation can take to execute.
You can set a single ``Timeout`` option on your ``Client`` instance to
specify the maximum amount of time that a single operation can take to execute.

Comment on lines 127 to 128
Set a timeout by using the ``SetTimeout()`` method or specifying the
``timeoutMS`` option in your connection URI string. ``Database``, ``Collection``,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Set a timeout by using the ``SetTimeout()`` method or specifying the
``timeoutMS`` option in your connection URI string. ``Database``, ``Collection``,
Set a timeout by calling the ``SetTimeout()`` method on a ``Client`` instance, or by specifying the
``timeoutMS`` option in your connection URI string. ``Database``, ``Collection``,

Comment on lines 128 to 131
``timeoutMS`` option in your connection URI string. ``Database``, ``Collection``,
``Session``, ``ChangeStream``, and ``Bucket`` instances elsewhere in
your code inherit the ``Timeout`` option from ``Client`` if you do not
set a different timeout in a Context for specific operations.
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: This sentence was a little bit hard to make sense of at first. I think splitting and rearranging some might help. Something like this maybe?

Suggested change
``timeoutMS`` option in your connection URI string. ``Database``, ``Collection``,
``Session``, ``ChangeStream``, and ``Bucket`` instances elsewhere in
your code inherit the ``Timeout`` option from ``Client`` if you do not
set a different timeout in a Context for specific operations.
``timeoutMS`` option in your connection URI string. By default, all ``Database``, ``Collection``,
``Session``, ``ChangeStream``, and ``Bucket`` instances elsewhere in
your code inherit the ``Timeout`` option from ``Client``. However, you can
set a different timeout for specific operations in the operation's Context.

Single Timeout Setting
----------------------

You can set a single ``Timeout`` option on your ``Client`` instance to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should "Timeout" be monospaced here? It seems like it's being used as a parameter/option name, but looking at the examples below it doesn't look like we actually have an option named "Timeout" that you can set.

Copy link
Collaborator Author

@rustagir rustagir Aug 7, 2024

Choose a reason for hiding this comment

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

The SetTimeout() method sets the Timeout struct field of the Client options struct.
Do you think this should be clarified in the doc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh I see, that makes more sense. I think it's probably okay then, people more familiar with Go will likely understand that

Comment on lines 133 to 136
If you pass a Context into an operation with a deadline, the driver uses
that Context deadline for the operation. If the context does not have a
deadline, the driver derives a new Context from the given Context by using
the ``Timeout`` option set on the ``Client``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be due to me not being very familiar with Go, but this paragraph is confusing to me. It seems a bit jarring to be talking about setting timeouts, but then the next paragraph is talking about using Context deadlines. Is a "deadline" the same thing as a timeout? If so I think we should stick with that wording. Happy to workshop some ways to rephrase this once I have a better understanding of what it's trying to say.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will clean this up (old text)

Comment on lines 168 to 169
The following example shows how you can set a single timeout with the
URI option and execute an operation that inherits this setting:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The following example shows how you can set a single timeout with the
URI option and execute an operation that inherits this setting:
The following example shows how to set a single timeout with the
URI option. It then executes an operation that inherits this timeout.

Comment on lines 183 to 185
The following example shows how you can set the ``Timeout`` option on a
``Client`` instance, then set a different timeout for an insert option
within the Context passed to the ``InsertOne()`` method:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The following example shows how you can set the ``Timeout`` option on a
``Client`` instance, then set a different timeout for an insert option
within the Context passed to the ``InsertOne()`` method:
The following example shows how to set the ``Timeout`` option on a
``Client`` instance. It then sets a different timeout
within the Context passed to the ``InsertOne()`` method:

Comment on lines 171 to 178
.. code-block:: go

uri := "mongodb://user:[email protected]:27017/?timeoutMS=5000"

client, err := mongo.Connect(options.Client().ApplyURI(uri))

...
coll.InsertOne(context.Background(), doc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: Highlight line 1. And maybe line 7?

Copy link
Collaborator

@jordan-smith721 jordan-smith721 left a comment

Choose a reason for hiding this comment

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

LGTM w/ one typo fix

source/fundamentals/connections/connection-options.txt Outdated Show resolved Hide resolved
Single Timeout Setting
----------------------

You can set a single ``Timeout`` option on your ``Client`` instance to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh I see, that makes more sense. I think it's probably okay then, people more familiar with Go will likely understand that

@rustagir rustagir requested a review from prestonvasquez August 7, 2024 15:07

.. note:: Retries under Timeout Specification

When using default settings, if you set a ``Timeout`` option on your ``Client``
Copy link
Collaborator

Choose a reason for hiding this comment

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

This behavior is true for a client-level or operation-level (context.Context) CSOT. I would also clarify that this only happens if the server returns a "retryable" error, such as a socket exception.

https://github.com/mongodb/specifications/blob/master/source/client-side-operations-timeout/client-side-operations-timeout.md#retryability

opts := options.Client().SetTimeout(5 * time.Second)
client, err := mongo.Connect(opts)

Connection String Option
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest specifying that the timeoutMS query parameter is a client-level timeout.

...
coll.InsertOne(context.Background(), doc)

Multiple Timeouts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest renaming this section to "Operation Timeout"

Comment on lines 194 to 195
opts := options.Client().SetTimeout(5 * time.Second)
client, err := mongo.Connect(opts)
Copy link
Collaborator

@prestonvasquez prestonvasquez Aug 8, 2024

Choose a reason for hiding this comment

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

I don't think it's necessary to show this. For an operation-level timeout, it doesn't matter if a client-timeout is set or not, the operation timeout takes priority. Suggest removing this line and updating the description of this example:

The following example shows how to set an operation-level timeout, which 
takes priority over a client-level timeout if one is defined:

@rustagir rustagir requested a review from prestonvasquez August 9, 2024 14:31
@prestonvasquez prestonvasquez removed the request for review from matthewdale August 12, 2024 16:11
@rustagir rustagir merged commit c26a559 into mongodb:master Aug 12, 2024
2 checks passed
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