Clarify zero-case connectTimeoutMS in SDAM setupConnection PC #1496
Conversation
prestonvasquez
requested review from
tom-selander and
ShaneHarvey
and removed request for
a team
| set connection timeout to connectTimeoutMS | ||
|
|
||
| if connectTimeoutMS != 0: | ||
| set connection timeout to connectTimeoutMS |
There was a problem hiding this comment.
At first glance this check seems a little too specific for pseudocode. I imagine this case being handled by the "set connection timeout to connectTimeoutMS" line. What's the motivation for this change?
There was a problem hiding this comment.
I was thrown off by the fact that we are specific in the handling of connectTimeoutMS in the streaming case on L782
It took some time to determine what to do with the zero case when establishing a connection for server monitoring, as the Go Driver doesn't currently apply any timeout. I think this socket timeout section of the spec covers my understanding of what to do:
Clients MUST use connectTimeoutMS as the timeout for the connection handshake.
When connectTimeoutMS=0, the timeout is unlimited and MUST remain unlimited
Looking closer, I think we would want to apply the if block to this line as well: https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-monitoring.rst#L903
Or remove the block on L782, either way I think we should be consistent.
There was a problem hiding this comment.
Okay I'm open to those changes if you'd like to make them.
| @@ -1231,6 +1233,7 @@ Changelog | |||
| :2022-11-17: Add minimum RTT tracking and remove 90th percentile RTT. | |||
| :2023-10-05: Add serverMonitoringMode and default to the polling protocol on FaaS. | |||
| Clients MUST NOT use dedicated connections to measure RTT when using the polling protocol. | |||
| :2024-01-17: Clarify zero-case connectTimeoutMS in SDAM setupConnection. | |||
There was a problem hiding this comment.
I don't think this change needs a entry since we're not changing spec requirements here.
|
@prestonvasquez are you planning to update this PR? |
Please complete the following before merging:
clusters, and serverless).
We can't use a 0 connectTimeoutMS when creating a socket for monitoring, since that will result in a timeout error. IIUC we should not set a timeout, described here. This should be reflected in the psuedocode. This update is validated by this unified spec test:
specifications/source/server-discovery-and-monitoring/tests/unified/connectTimeoutMS.yml
Line 23 in f1bbc24