-
Notifications
You must be signed in to change notification settings - Fork 916
GODRIVER-3582 Add custom options to client bulkWrite. #2172
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
base: master
Are you sure you want to change the base?
Conversation
🧪 Performance ResultsCommit SHA: 4dd4232The following benchmark tests for version 68a75c2ad3686000077f5b6b had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change ReportNo changes found! |
b497a08
to
52442f5
Compare
52442f5
to
4fd629f
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.
Pull Request Overview
This PR adds support for two new internal options to the client bulkWrite functionality: a boolean flag for bypassing empty timestamp replacement and a generic callback function for adding undocumented parameters to commands.
- Adds
bypassEmptyTsReplacement
boolean option for client bulkWrite operations - Implements a
commandCallback
function option to allow dynamic command modifications - Updates command building logic to incorporate these new options
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
x/mongo/driver/xoptions/options.go | Adds option parsing for new bypassEmptyTsReplacement and commandCallback parameters |
mongo/client_bulk_write.go | Updates clientBulkWrite struct and command building logic to support new options |
mongo/client.go | Extracts and sets new options from internal options during client bulkWrite execution |
internal/integration/client_test.go | Adds integration test for commandCallback functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -101,6 +102,24 @@ func SetInternalClientBulkWriteOptions(a *options.ClientBulkWriteOptionsBuilder, | |||
opts.Internal = optionsutil.WithValue(opts.Internal, key, b) | |||
return nil | |||
}) | |||
case "bypassEmptyTsReplacement": |
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.
Should we skip adding bypassEmptyTsReplacement
, instead deferring to the user to set it using the command callback?
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.
Despite being internal, I recommend retaining helper functions like bypassEmptyTsReplacement
since the command callback shouldn't be used frequently.
mongo/client_bulk_write.go
Outdated
if bw.bypassEmptyTsReplacement != nil { | ||
dst = bsoncore.AppendBooleanElement(dst, "bypassEmptyTsReplacement", *bw.bypassEmptyTsReplacement) | ||
} | ||
if bw.commandCallback != nil { |
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.
IIUC this callback is designed to be a final mutator and so is kind of a dangerous addition to the code since it has to always come last in the function returned by newCommand. At the very least we should note that in a comment.
For long-term maintenance and to avoid bugs, it might be best to enforce this behavior. Perhaps we could use a builder pattern:
type clientBulkWrite struct {
builder commandBuilder
}
type commandBuilder struct {
base func([]byte, description.SelectedServer) ([]byte, error)
callback func([]byte, description.ServerSelector) ([]byte, error)
}
func (b commandBuilder) build(dst []byte, desc description.SelectedServer) ([]byte, error) {
out, err := b.base(dst, desc)
if err != nil {
return nil, err
}
// The callback is a final mutator.
if b.callback != nil {
out, err := b.callback(out, desc)
if err != nil {
return nil, err
}
}
return out, nil
}
Then in newCommand()
:
bw.builder.base = func(dst []byte, desc description.SelectedServer) ([]byte, error) {
// ...
}
return bw.builder.build
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.
The implementation looks good. 👍 We should wait to hear back if it accomplishes the mongosync use case before merging.
GODRIVER-3582
Summary
Background & Motivation