-
Notifications
You must be signed in to change notification settings - Fork 916
GODRIVER-3659 Add awaitMinPoolSize to clientEntity constructor #2196
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-3659 Add awaitMinPoolSize to clientEntity constructor #2196
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 the awaitMinPoolSize
option to the unified test runner's client entity constructor to improve test stability by warming connection pools before tests run.
- Updates supported schema version from 1.22 to 1.26
- Adds
awaitMinPoolSize
field toentityOptions
struct with documentation - Implements connection pool warming logic that waits for minimum pool size to be reached
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
internal/integration/unified/schema_version.go | Updates supported schema version to 1.26 |
internal/integration/unified/entity.go | Adds awaitMinPoolSize boolean field to entityOptions |
internal/integration/unified/client_entity.go | Implements pool warming functionality and integrates it into client creation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if entityOptions.AwaitMinPoolSize && clientOpts.MinPoolSize != nil && *clientOpts.MinPoolSize > 0 { | ||
if err := awaitMinimumPoolSize(ctx, entity, *clientOpts.MinPoolSize); err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
entity.Client = client |
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 awaitMinimumPoolSize
function is called with entity
before entity.Client
is assigned (line 235). This means the entity doesn't have a valid client when the function tries to check connection events, which could cause the function to behave incorrectly.
if entityOptions.AwaitMinPoolSize && clientOpts.MinPoolSize != nil && *clientOpts.MinPoolSize > 0 { | |
if err := awaitMinimumPoolSize(ctx, entity, *clientOpts.MinPoolSize); err != nil { | |
return nil, err | |
} | |
} | |
entity.Client = client | |
entity.Client = client | |
if entityOptions.AwaitMinPoolSize && clientOpts.MinPoolSize != nil && *clientOpts.MinPoolSize > 0 { | |
if err := awaitMinimumPoolSize(ctx, entity, *clientOpts.MinPoolSize); err != nil { | |
return nil, err | |
} | |
} |
Copilot uses AI. Check for mistakes.
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 doesn't matter since the pool event monitor was defined before client construction.
This branch must await mongodb/specifications#1834 and the subsequent specifications bump before merging. |
🧪 Performance ResultsCommit SHA: be25c6dThe following benchmark tests for version 68c454c96d994a0007fa14a5 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! |
6358fe0
to
f6da88f
Compare
f6da88f
to
c2611f2
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! 👍
GODRIVER-3659
Summary
Add
awaitMinPoolSize
to the unified test runner.Background & Motivation
Warming clients should make CSOT tests less flakey.