GODRIVER-3659 Filter CMAP/SDAM events for awaitMinPoolSizeMS #2235
Conversation
There was a problem hiding this comment.
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.
🧪 Performance ResultsCommit SHA: 8619a32The following benchmark tests for version 69264177bcbd2f0007cdc247 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! |
prestonvasquez
force-pushed
the
ci/godriver-3659-await-min-pool-size-in-ust-new
branch
from
acbd1d0 to
4110738
Compare
prestonvasquez
added
the
review-priority-low
| return fmt.Errorf("timed out waiting for client to reach minPoolSize") | ||
| case <-ticker.C: | ||
| if uint64(entity.getEventCount(connectionReadyEvent)) >= minPoolSize { | ||
| entity.eventSequencer.setCutoff() |
There was a problem hiding this comment.
An alternative to this is to re-initialize the event containers here. While this is a much simpler solution, it could be buggy depending if we need to re-initialize event logic for some other reason somewhere else. The eventSequencer future-proofs this issue.
|
Closing to focus on a simpler solution for now. |
| seqSlice = c.eventSequencer.seqByEventType[eventType] | ||
| } | ||
|
|
||
| localSeqs := append([]int64(nil), seqSlice...) |
There was a problem hiding this comment.
Optional: Use slices.Clone.
| localSeqs := append([]int64(nil), seqSlice...) | |
| localSeqs := slices.Clone(seqSlice) |
| cutoffAfter: 2, | ||
| setupEvents: func(c *clientEntity) { | ||
| recordPoolEvent(c) | ||
| recordPoolEvent(c) | ||
| // Cutoff will be set here (after event 2) | ||
| recordPoolEvent(c) |
There was a problem hiding this comment.
Setting the cutoff point by manipulating the internal state of eventSequencer makes this test dependent on the internal implementation. Instead, call c.eventSequencer.setCutoff() at the expected point in the sequence in the setupEvents func.
E.g.
setupEvents: func(c *clientEntity) {
recordPoolEvent(c)
recordPoolEvent(c)
c.eventSequencer.setCutoff()
recordPoolEvent(c)
recordPoolEvent(c)
recordPoolEvent(c)
// ...| func (es *eventSequencer) recordEvent(eventType monitoringEventType) { | ||
| next := es.counter.Add(1) | ||
|
|
||
| es.mu.Lock() | ||
| es.seqByEventType[eventType] = append(es.seqByEventType[eventType], next) |
There was a problem hiding this comment.
Instead of storing every sequence number per event type, can we only store the index when the cutoff happened?
E.g.
func (es *eventSequencer) recordEvent(...) {
es.mu.Lock()
defer es.mu.Unlock()
if !es.isCutoff {
es.cutoffByEventType[eventType]++
}
}
func (es *eventSequencer) setCutoff() {
es.mu.Lock()
defer es.mu.Unlock()
es.isCutoff = true
}That would also simplify filterEventsBySeq because you just get a subslice index:
func filterEventsBySeq[T any](...) []T {
// ...
cutIdx := c.eventSequencer.cutoffByEventType[eventType]
return localEvents[cutIdx:]
}There was a problem hiding this comment.
While this approach is simpler, it only supports a single cutoff per test. If the unified spec adds scenarios with multiple cutoffs (e.g., awaitMinPoolSizeMS (implicit cutoff) → run X → cutoff at N (either implicit or explicit) → run Y), we’d have to refactor back to the original design.
| func (es *eventSequencer) recordPooledEvent() { | ||
| next := es.counter.Add(1) | ||
|
|
||
| es.mu.Lock() | ||
| es.poolSeq = append(es.poolSeq, next) |
There was a problem hiding this comment.
Is there a reason this has to be a separate counter? Can this call es.recordEvent(poolAnyEvent)?
| case <-awaitCtx.Done(): | ||
| return fmt.Errorf("timed out waiting for client to reach minPoolSize") | ||
| case <-ticker.C: | ||
| if uint64(entity.getEventCount(connectionReadyEvent)) >= minPoolSize { |
There was a problem hiding this comment.
The spec says that awaitMinPoolSizeMS must wait for each connection pool to reach minPoolSize, but this seems to wait for the total number of connections across all pools. Is there a way to wait for each connection pool to reach minPoolSize?
…hub.com:prestonvasquez/mongo-go-driver into ci/godriver-3659-await-min-pool-size-in-ust-new
2 participants
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.
Additionally, the clientEntity requires that the following fields not be reset:
This guarantee would have to be removed with the naive approach, at least for eventsCount.
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: