-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(CSOT) - feature branch #4095
base: main
Are you sure you want to change the base?
Conversation
5688ba8
to
b878c6c
Compare
readPreference: options?.readPreference, | ||
timeoutMS: options?.timeoutMS ?? this.s.db.timeoutMS |
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.
NOTE FOR REVIEWERS: Needed to do this to ensure that unit tests using ping work
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.
left some small comments. I'm out this week, so don't consider them blocking!
src/sdam/server.ts
Outdated
return await Promise.race([ | ||
finalOptions.operationTimeout, | ||
conn.command(ns, cmd, finalOptions) | ||
]); |
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.
I don't recall any CSOT requirement that states we should time out the entirety of command execution at the connection layer. Instead, each step of connection-layer command execution has its own rules (your PR description mentions that this PR does not implement this logic.
Is this only added to facilitate testing? Or is there a CSOT requirement I'm missing?
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.
Good catch here. Meant to remove this change, but missed it.
Bookkeeping: |
test/integration/client-side-operations-timeout/client_side_operations_timeout.unit.test.ts
Show resolved
Hide resolved
@aditi-khare-mongoDB @nbbeeken should we conditionally clear the timeout on success/a non-timeout failure based on whether or not we created the timeout inside the function that we race the timeout with? |
3591368
to
a645d9f
Compare
d224fff
to
c3f31da
Compare
c3f31da
to
4fd4b24
Compare
Description
What is changing?
New Error
MongoOperationTimeoutError
class that is thrown when a CSOT timeout is encounteredChanges to
Timeout
Timeout.throwIfExpired()
methodTimeout.remainingTime
getter methodUpdates to
AbstractOperation
timeout
fieldtimeout
is set at construction if thetimeoutMS
option is providedImplementing CSOT behaviour for server selection
Topology.selectServer
to accept atimeout
option which it will use determine whether it has timed out when defined. Otherwise, constructs aTimeout
using theserverSelectionMS
option as beforeTopology.selectServer
to throw aMongoOperationTimeoutError
on timeout whenoptions.timeout
is provided and retain previous error behaviour otherwise.Topology._connect
to pass downtimeout
toServer.command
call used to execute ping on first connectionImplementing CSOT behaviour for connection checkout
Server.command
to accepttimeout
option.ConnectionPool.checkOut
to accepttimeout
optionserverSelectionTimeoutMS
is greater than the duration on thetimeout
, otherwise, computes the time elapsed since server selection completed and creates timeout for theserverSelectionTimeoutMS
deadlineTest changes
Misc changes
resolveOptions
to handletimeoutMS
option propagationcsotMin
helper method that implements the CSOT min algorithm described hereIs there new documentation needed for these changes?
What is the motivation for this change?
Release Highlight
Fill in title or leave empty for no highlight
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript