-
Notifications
You must be signed in to change notification settings - Fork 244
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
DRIVERS-2798 Drivers do not gossip $clusterTime on SDAM commands #1735
base: master
Are you sure you want to change the base?
Conversation
@@ -710,7 +710,8 @@ expiration. | |||
|
|||
## Gossipping the cluster time | |||
|
|||
Drivers MUST gossip the cluster time when connected to a deployment that uses cluster times. | |||
Drivers MUST gossip the cluster time when connected to a deployment that uses cluster times. However, drivers MUST NOT | |||
gossip cluster time on SDAM 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.
Should this link to the SDAM spec in some way, so that it's clear what SDAM commands refers to?
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.
I followed the pattern in the load balancer spec and added "SDAM" to the terms section.
@@ -238,6 +238,20 @@ and configure a `MongoClient` with default options. | |||
- Attempt to send a write command to the server (e.g., `insertOne`) with the explicit session passed in | |||
- Assert that a client-side error is generated indicating that sessions are not supported | |||
|
|||
### 20. Drivers do not gossip `$clusterTime` on SDAM 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.
I'm unsure about this test. Previously we didn't have a test that asserts that cluster time is gossiped by SDAM commands. And now we have a test that it is not. When we fixed this issue in the java driver we didn't end up adding an integration test for it, because it was fixed by design: in the Java driver in order to gossip cluster time you need access to the client-wide instance of ClusterClock
. In the fix we just removed the ClusterClock
from the server monitor, so there in way it could unintentionally gossip.
That said, for other drivers I guess this test might be a useful one, so if you want to leave it I'm ok with that.
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.
I'd prefer to keep the test. It does provide some value and it at least saves implementors' time trying to answer "how should I test this? Can I even test it?".
Please complete the following before merging:
clusters, and serverless).