-
Notifications
You must be signed in to change notification settings - Fork 918
GODRIVER-3288 Stop gossiping $clusterTime on SDAM commands. #2150
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
Conversation
API Change ReportNo changes found! |
111c4bc
to
5c9f226
Compare
5d5a41a
to
76cd8a3
Compare
76cd8a3
to
80da069
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 removes cluster clock gossiping from Server Discovery and Monitoring (SDAM) commands, specifically the "hello" command used for heartbeats and initial server discovery. The change ensures that $clusterTime
is not included in SDAM operations to comply with MongoDB driver specifications.
- Removed cluster clock parameter from Hello operation constructors
- Added null checks to prevent cluster time operations when no clock is provided
- Added integration test to verify SDAM commands no longer gossip
$clusterTime
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
x/mongo/driver/topology/topology_options.go | Removed cluster clock from Hello operation configuration |
x/mongo/driver/topology/server.go | Removed cluster clock from heartbeat Hello operations |
x/mongo/driver/operation/hello.go | Removed ClusterClock method and field from Hello struct |
x/mongo/driver/operation.go | Added null checks for cluster clock operations |
x/mongo/driver/auth/auth.go | Removed cluster clock from authentication Hello operations |
internal/integration/sessions_test.go | Added test to verify SDAM commands don't gossip cluster time |
internal/integration/mtest/mongotest.go | Added null check before client disconnect |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
} | ||
|
||
// ClusterClock sets the cluster clock for this operation. | ||
func (h *Hello) ClusterClock(clock *session.ClusterClock) *Hello { |
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 we still want to gossip cluster times when establishing connections.
10961c2
to
375f935
Compare
op.updateClusterTimes(res) | ||
// When a cluster clock is given, update cluster/operation time and recovery tokens before handling the error | ||
// to ensure we're properly updating everything. | ||
if op.Clock != 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.
Do we need this block? Wouldn't op.updateClusterTimes(res)
be a no-op if op.Click == nil
for hello commands?
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.
We need to stop updating the cluster time in session too, so L1709 in addClusterTime() will not append a “$clusterTime”.
heartbeatStarted := make(chan struct{}) | ||
heartbeatSucceeded := make(chan struct{}) |
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.
Suggest buffering these channels and adding a wait function:
wait := func(mt *mtest.T, ch <-chan struct{}, label string) {
mt.Helper()
select {
case <-ch:
case <-time.After(5 * time.Second):
mt.Fatalf("timed out waiting for %s", label)
}
}
wait(mt, heartbeatStarted, "heartbeat started")
wait(mt, heartbeatSucceeded, "heartbeat succeeded")
GODRIVER-3288
Summary
Stop gossiping $clusterTime on SDAM commands.
Background & Motivation