-
Notifications
You must be signed in to change notification settings - Fork 918
GODRIVER-3659 Filter CMAP/SDAM events for awaitMinPoolSizeMS #2235
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?
GODRIVER-3659 Filter CMAP/SDAM events for awaitMinPoolSizeMS #2235
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 implements event filtering for awaitMinPoolSizeMS in the unified test runner to comply with the specification requirement that CMAP and SDAM events occurring during connection pool initialization should be ignored. The implementation uses a sequence-based filtering approach where events are assigned monotonically increasing sequence numbers, and a cutoff is set after pool initialization completes.
Key Changes:
- Replaced boolean
awaitMinPoolSizefield withawaitMinPoolSizeMSinteger field to specify timeout duration - Introduced
eventSequencerto track event ordering via sequence numbers and filter events below a cutoff threshold - Modified event processing functions to record sequence numbers for all CMAP and SDAM events
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/integration/unified/entity.go | Updated entityOptions to use awaitMinPoolSizeMS with timeout duration instead of boolean flag |
| internal/integration/unified/client_entity.go | Added eventSequencer type and filtering logic; updated awaitMinimumPoolSize to accept timeout parameter and set cutoff after pool initialization |
| internal/integration/unified/event_verification.go | Modified event verification functions to use filterEventsBySeq for filtering CMAP and SDAM events |
| internal/integration/unified/client_entity_test.go | Added comprehensive unit tests for eventSequencer functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if c.eventSequencer.cutoff == 0 { | ||
| return events | ||
| } | ||
|
|
||
| var filtered []T | ||
| for i, evt := range events { | ||
| if seqSlice[i] > c.eventSequencer.cutoff { | ||
| filtered = append(filtered, evt) | ||
| } | ||
| } |
Copilot
AI
Nov 8, 2025
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.
Potential index out of bounds panic if seqSlice length doesn't match events length. Add a length check before the loop or validate that i < len(seqSlice) in the loop condition.
| return false | ||
| } | ||
|
|
||
| return es.seqByEventType[eventType][index] <= es.cutoff |
Copilot
AI
Nov 8, 2025
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.
Potential index out of bounds panic if the index is beyond the slice length. Add bounds checking before accessing es.seqByEventType[eventType][index].
| return es.seqByEventType[eventType][index] <= es.cutoff | |
| seqs, ok := es.seqByEventType[eventType] | |
| if !ok || index < 0 || index >= len(seqs) { | |
| return false | |
| } | |
| return seqs[index] <= es.cutoff |
🧪 Performance ResultsCommit SHA: 95e4d63The following benchmark tests for version 690e89e64dcf7a0007e61054 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! |
GODRIVER-3659
Summary
This PR implements event filtering for
awaitMinPoolSizeMSin the unified test runner, as specified in the unified test format specification.A naive implementation would clear specific event arrays after initialization:
However, if we add a new CMAP or SDAM event type in the future, we must remember to update this clearing block. Forgetting to do so means initialization events leak into test assertions, causing false failures.
The
eventSequencerassigns a monotonically increasing sequence number to each CMAP and SDAM event as it's recorded. After pool initialization completes, we capture the current sequence as a cutoff. When verifying test expectations, we filter out any events with sequence numbers at or below the cutoff. This approach is future-proof because new event types automatically participate in filtering as long as they call the appropriate recording method in their event processor.Background & Motivation
PR #2196 added support for
awaitMinPoolSizeMSto the unified test runner, but was merged before the specification PR mongodb/specifications#1834 was finalized. As a result, the initial implementation used a simplified approach that doesn't match the final specification requirements.Per the spec, when
awaitMinPoolSizeMSis specified: