-
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? |
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.
looks pretty good
src/sdam/server.ts
Outdated
if (options.operationTimeout) { | ||
conn = await this.pool.checkOut({ timeout: options.operationTimeout }); | ||
} else { | ||
conn = await this.pool.checkOut(); | ||
} |
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 (options.operationTimeout) { | |
conn = await this.pool.checkOut({ timeout: options.operationTimeout }); | |
} else { | |
conn = await this.pool.checkOut(); | |
} | |
conn = await this.pool.checkOut({ timeout: options.operationTimeout }); |
TS supports just calling this because the timeout is optional
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 feel Warren's current code is easier to read (and easier for someone editing the code later to not accidentally make the code not CSOT spec-compliant) , but if we do end up going with this suggestion can we leave a clarifying comment?
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 am surprised because breaking this up into two calls to checkOut based on a condition that does matter is more to read without more meaningful context given. Whether or not timeout exists, there is no change to how checkOut is, practically, invoked because the typescript reports that field as optional.
I would actually take this further:
conn = await this.pool.checkOut(options);
Why do we need to make a new object here? passing through options should be fine right? Less branching paths the less there is to debug
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.
Can you elaborate on what may accidentally break the spec without a test warning us?
src/cmap/connection_pool.ts
Outdated
// Determine if we're using the timeout passed in or a new timeout | ||
if (options.timeout.duration > 0 || serverSelectionTimeoutMS > 0) { | ||
if ( | ||
csotMin(options.timeout.duration, serverSelectionTimeoutMS) === serverSelectionTimeoutMS |
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 is still and equals check, sorry I think we discussed it but didn't leave a comment, if duration is the same then we'll create a new timeout when we can use the existing one.
4993bbe
to
e3b7a63
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