-
Notifications
You must be signed in to change notification settings - Fork 246
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
DRIVERS-2868 Adjust getMore maxTimeMS Calculation for tailable awaitData Cursors #1749
base: master
Are you sure you want to change the base?
Conversation
source/client-side-operations-timeout/client-side-operations-timeout.md
Outdated
Show resolved
Hide resolved
source/client-side-operations-timeout/client-side-operations-timeout.md
Outdated
Show resolved
Hide resolved
source/client-side-operations-timeout/client-side-operations-timeout.md
Outdated
Show resolved
Hide resolved
"name": "iterateOnce", | ||
"object": "tailableCursor", | ||
"arguments": { | ||
"timeoutMS": 50 |
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.
In timeoutMS is refreshed for getMore if maxAwaitTimeMS is set
test case, timeoutMS
is set when the cursor is created. I suggest we follow the same approach here and the other test since not all drivers allow setting a timeout for each next()
call as an argument - it gets refreshed under the hood and i believe is supposed to behave the same in the drivers which additionally allow to override timeouMS
per next
() call.
For example, we could set timeoutMS to 150ms and maxAwaitTimeMS to 100ms like this:
{
"name": "createFindCursor",
"object": "collection",
"arguments": {
"filter": {},
"cursorType": "tailableAwait",
"batchSize": 1,
"timeoutMS": 150
"maxAwaitTimeMS": 100
},
"saveResultAsEntity": "tailableCursor"
},
Also, we configure a failpoint to block the find command for 60ms
and call iterateOnce()
. At this point, maxTimeMS
on the command should be ≤ 90ms, which is ≤ maxAwaitTimeMS
."
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.
@vbabanin It's not idiomatic for the Go Driver to use the timeoutMS applied to constructing a cursor to each next()
call. This would violate user expectations of context.Context. For example,
findCtx, findCancel := context.WithTimeout(context.Background(), 1*time.Second)
defer findCancel()
cur, _ := coll.Find(findCtx, bson.D{}) // User is applying a 1s timeout
// User does not apply a timeoutMS to the context passed to Next, internally
// applying a timeout here would violate expectations.
for cursor.Next(context.Background()) {
// ...
}
Tests such as timeoutMS is refreshed for getMore if maxAwaitTimeMS is set
are skipped in the Go Driver specifically because timeoutMS is not being applied at either the client or operation level. More than likely we will create a DRIVERS ticket to address this, see GODRIVER-3480 for more info. IIUC, we are likely to suggest putting the same timeout on both constructor and next. Would this work for Java?
operations:
- name: createFindCursor
object: *collection
arguments:
filter: {}
cursorType: tailableAwait
batchSize: 1
maxAwaitTimeMS: 50
timeoutMS: 100
saveResultAsEntity: &tailableCursor tailableCursor
# Iterate twice to force a getMore.
- name: iterateOnce
object: *tailableCursor
arguments:
timeoutMS: 100
- name: iterateOnce
object: *tailableCursor
arguments:
timeoutMS: 100
…imeout.md Co-authored-by: Viacheslav Babanin <[email protected]>
DRIVERS-2868
Please complete the following before merging:
clusters, and serverless).