-
Notifications
You must be signed in to change notification settings - Fork 918
GODRIVER-3616 Apply client-level timeout to tailable cursors #2174
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
GODRIVER-3616 Apply client-level timeout to tailable cursors #2174
Conversation
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.
Pull Request Overview
This PR adds support for client-level timeouts with Tailable Await Data (TAD) cursors by implementing a mechanism to apply client-level timeout when no context deadline is set.
- Adds client timeout handling to cursor operations for better timeout management
- Implements a functional options pattern for cursor configuration
- Integrates client timeout from collection to cursor initialization
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
mongo/cursor.go | Adds client timeout field and options pattern, implements timeout logic in cursor.next() |
mongo/collection.go | Passes client timeout to cursor creation |
internal/integration/cursor_test.go | Adds integration test for client-level timeout behavior with TAD cursors |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
API Change ReportNo changes found! |
🧪 Performance ResultsCommit SHA: 5c404fbThere were no significant changes to the performance to report for version 68b86cfcfc0e9b0007f95beb.
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
2c15fcd
to
4a9229b
Compare
3d2f9d9
to
dece3ac
Compare
type cursorOptions struct { | ||
clientTimeout time.Duration | ||
hasClientTimeout bool |
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.
Is there a reason to use a value+bool pair here rather than a pointer like in other options APIs in the Go Driver? Also, is there a generic "optional value" type that we can try using here since it's an unexported API?
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 prefer this solution to avoid referencing the value passed to withCursorOptionClientTimeout
:
func withCursorOptionClientTimeout(dur *time.Duration) cursorOption {
return func(opts *cursorOptions) {
opts.clientTimeout = dur // Changes to clientTimeout changes dur
}
}
Taking copy feels kind of awkward:
func withCursorOptionClientTimeout(dur *time.Duration) cursorOption {
return func(opts *cursorOptions) {
if dur != nil {
durCopy := *dur
opts.clientTimeout = &durCopy
}
}
}
Not passing the pointer requires extra setup in the collection methods.
IIRC you have a global solution, if you open a GODRIVER ticket I will add it as a TODO here.
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.
Created GODRIVER-3650 to track that suggestion.
3bbfbf6
to
e866d92
Compare
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 good! 👍
6fa9b35
to
8949cfc
Compare
8949cfc
to
97b830a
Compare
b36f082
to
1029688
Compare
1029688
to
5c404fb
Compare
GODRIVER-3616
Summary
Apply client-level timeouts for default ITERATION_MODE for tailable/awaitData cursors.
Background & Motivation
Detailed steps to reproduce the problem?
According to the CSOT specifications, the timeoutMS value must apply to cursor iteration methods such as
cursor.Next()
: