DRIVERS-1607: Snapshot reads on Secondaries #1022

BorisDog · 2021-06-16T10:51:21Z

No description provided.

BorisDog · 2021-06-16T10:54:25Z

source/sessions/tests/unified/snapshot-sessions.json

@@ -0,0 +1,706 @@
+{


snapshot-sessions.yml will be added after json test version will be finalized.

FYI we usually develop (and review) the yaml version because it's easier to read and supports comments and anchors to reduce the verbosity. Also the json file is auto-generated from the yaml.

ajdavis

I'm so happy that we're choosing the ClientSession API!

source/read-write-concern/read-write-concern.rst

source/sessions/snapshot-sessions.rst

ajdavis · 2021-06-16T12:05:38Z

source/sessions/snapshot-sessions.rst

+this property is false.
+
+Snapshot reads and causal consistency are mutually exclusive. Therefore if ``isSnapshot`` is set to true,
+``causalConsistency`` property is set to false. Client MUST throw an Error if both ``isSnapshot`` and ``causalConsistency`` are set to true.


The server permits a snapshot read to begin with afterClusterTime so it's causally after some previous operation. Within a snapshot read, of course, you can't be causally consistent. This API doesn't expose the "begin a snapshot read causally after an operation" feature. That might be fine, and we could enhance the API later if it turns out that users want that feature. I want to confirm it's our intention to omit the feature now.

@jyemin @ajdavis
Making sure that we want drivers forbid requesting both causalConsistency and isSnapshot (need to add a test for that).
I lean to raising an error in such case in driver, as otherwise due to drivers possible implementation differences it could be that some would send both atClusterTime and afterClusterTime, and some only one of the fields. Which will result in inconsistent behavior.

Let's start with making it an error. It's easier to go from error to not error than the other way around.

Just FYI, libmongoc defines mongoc_session_opt_t with a field flags that is an enum mongoc_session_flag_t. I'll add another variant for isSnapshot, and by nature of the enum, there will be no way to specify causalConsistency and isSnapshot simultaneously. I'll still throw an error if users try to, but worth noting that if other drivers use a similar enum, users won't be able to do this anyways.

source/sessions/snapshot-sessions.rst

BorisDog · 2021-06-16T10:56:38Z

source/sessions/snapshot-sessions.rst

+    * assert that the command does not have an ``atClusterTime``.
+
+2.  | Subsequent snapshot reads on a ``ClientSession`` should read from the snapshot of the first read in that session.
+    | Test SHOULD introduce two variations one for primaries and additional for secondary reads


@jyemin currently with test against secondaries only, should we duplicate the tests to run on primary as well?

I think that would be more a test of the server than of drivers, so I don't think it's necessary. What do you think, @ajdavis?

Yeah, this is tested on the server in snapshot_reads.js, no need for drivers to do it.

source/sessions/tests/unified/snapshot-sessions.json

source/sessions/snapshot-sessions.rst

source/read-write-concern/read-write-concern.rst

source/read-write-concern/tests/README.rst

source/sessions/snapshot-sessions.rst

alcaeus · 2021-06-17T08:30:46Z

source/sessions/snapshot-sessions.rst

+Snapshot Reads Specification
+================================
+
+:Spec Title: Snapshot Reads Specification (See the registry of specs)


I don't think we need to call out a registry of specs here

Suggested change

:Spec Title: Snapshot Reads Specification (See the registry of specs)

:Spec Title: Snapshot Reads Specification

Not sure what's the convention here as this referral appears in causal-consistency.rst and driver-sessions.rst.
Removed for now.

I believe those are leftovers from the templates. I'll give the specs a quick look to clean these up. Thanks for pointing out other instances.

source/sessions/snapshot-sessions.rst

source/sessions/tests/unified/snapshot-sessions.json

alcaeus · 2021-06-17T12:22:14Z

source/unified-test-format/unified-test-format.rst

    specifications:

    - `Causal Consistency <../causal-consistency/causal-consistency.rst#sessionoptions-changes>`__
+    - `Snapshot Reads <../sessions/snapshot-sessions.rst#sessionoptions-changes>`__


I forgot to mention this, but you should add a make target for session tests in the unified test runner Makefile. This ensures all unified session tests are checked against the schema during the CI build.

Great, thanks! Added, and will verify when yml tests are added.

jyemin · 2021-06-17T14:34:04Z

source/sessions/tests/unified/snapshot-sessions.json

+    {
+      "minServerVersion": "5.0",
+      "topologies": [
+        "sharded-replicaset"


I think this should be:

topologies: [ "replicaset", "sharded-replicaset" ]

Thanks, fixed.

jyemin · 2021-06-17T14:43:22Z

source/sessions/tests/unified/snapshot-sessions.json

+        "client": "client0",
+        "sessionOptions": {
+          "isSnapshot": true,
+          "snapshotTimestamp": 0


I'm not clear what your intention is here. In the spec this option is not defined.

I've marked this test as optional, for drivers that can alter snapshotTimestamp field internally.

Not sure what's the best approach here: Mandate drivers to provide functionality to set snapshotTimestamp or extract this test as optional prose test.

jyemin · 2021-06-17T14:55:02Z

source/sessions/tests/unified/snapshot-sessions.json

+              "commandStartedEvent": {
+                "command": {
+                  "find": "collection0",
+                  "readConcern": {


Let's add commandStartedEvent assertions to all the tests, demonstrating that atClusterTime is included when appropriate.

That's problematic as we don't have a way to obtain actual atClusterTime.

Done with $$exists.

jyemin · 2021-06-17T14:56:20Z

source/sessions/tests/unified/snapshot-sessions.json

+      }
+    },
+    {
+      "session": {


Is there really a need for all these session objects? Each test runs in isolation, so a new session instance is created for each test. So there is no re-use issues between tests. I suspect we can have just one session with an isSnapshot: true option.

No need, removed.

- atClusterTime in Distinct command result documented

jyemin · 2021-06-17T17:14:32Z

source/sessions/tests/unified/snapshot-sessions.json

+      ]
+    },
+    {
+      "description": "First snapshot read does send atClusterTime",


I think it should be does not

jyemin · 2021-06-17T21:07:32Z

source/sessions/snapshot-sessions.rst

+.. code:: typescript
+
+    interface ClientSession {
+        Optional<BsonTimestamp> snapshotTimestamp;


Now I think about it, it's not clear why we need this to be a public property if there is no way to initialize a new ClientSession with its value.

Sounds right, currently there is not need for snapshotTimestamp in public API. Removed.

As ClientSession.snapshotTimestamp is likely to be introduced later, drivers still need this field as part of ClientSession enitity as mentioned here.

jyemin · 2021-06-17T21:11:15Z

source/sessions/snapshot-sessions.rst

+2. aggregate
+3. distinct
+
+Snapshot read commands errors


I'm not convinced its necessary to specify this, since retryable reads are only enabled via an allow-list anyway.

Thanks, removed.

jyemin · 2021-06-17T21:11:57Z

source/sessions/snapshot-sessions.rst

+
+Drivers MUST NOT retry errors in SnapshotError category for snapshot reads if ``atClusterTime`` is supplied.
+
+Test Plan


Check with others, but I think this whole section should move to the README.rst in the tests subdirectory.

I agree; seems like the normal place for prose tests.

jyemin · 2021-06-17T21:13:18Z

source/sessions/snapshot-sessions.rst

+replica set or a sharded cluster supports snapshot reads.
+The server ``minSnapshotHistoryWindowInSeconds`` parameter SHOULD be configured to match the test execution time.
+
+1.  | The first read in a snapshot session must not send ``atClusterTime``


This only needs to be included for prose tests. You don't have to duplicate what's already specified in YAML.

Done. Thanks.

benjirewis

Barring tests with distinct (see my other comment), spec tests seem to pass in C. Will implement prose test and re-review.

benjirewis · 2021-06-23T19:00:35Z

source/sessions/tests/unified/snapshot-sessions.json

+        "id": "session0",
+        "client": "client0",
+        "sessionOptions": {
+          "isSnapshot": true


Can we specify "causalConsistency": false here and on session1? causalConsistency is true by default which will cause an error with "isSnapshot": true.

The default causalConsistency value is "not supplied". So in that case it should be translated to !isSnapshot (now it's translated to true)?
I think drivers should account for this case, as we don't want mandate users to always specify "causalConsistency": false if snapshot session is requested.

I agree that specifying causalConsistency = false just for isSnapshot = true should not be a burden on the users.

But if a user (or a test) sets isSnapshot to true, drivers should set causalConsistency to false? I suppose I'm confused about when drivers should explicitly error because of simultaneously true isSnapshot and causalConsistency values, and when drivers should quietly switch one or the other off.

You are right, this point is not clear.
I suggest that causalConsistency defaults to true only if no snapshot where requested.

benjirewis · 2021-06-23T19:01:42Z

source/sessions/tests/unified/snapshot-sessions.json

+      ]
+    },
+    {
+      "description": "Distinct operation with snapshot",


I cannot run this test and Mixed operation with snapshot in libmongoc since we don't have a dedicated distinct helper, so I can't verify that these work, but they look good to me.

rstam

LGTM

ShaneHarvey · 2021-06-23T22:55:46Z

source/sessions/snapshot-sessions.rst

+    specification defines how sessions are used to implement snapshot reads.
+
+Snapshot reads
+    Reads with readconcern level ``snapshot`` that occur outside of transactions on


readconcern -> read concern

ShaneHarvey · 2021-06-23T22:58:59Z

source/sessions/snapshot-sessions.rst

+MongoClient changes
+===================
+
+There are no API changes to ``MongoClient`` to support snapshot reads.


It's a bit confusing to have "MongoClient/MongoDatabase/MongoCollection changes" sections when there aren't any changes at all. Can we remove those sections? We could add one sentence to the rationale which says something like "This spec does not require any client, db, collection API changes."

I agree, we might not need even that cliet/db/collection section.
@rstam do you think we could skip those no-changes sections?

ShaneHarvey · 2021-06-23T23:00:16Z

source/sessions/snapshot-sessions.rst

+snapshot reads are implemented by:
+
+1. Saving the ``atClusterTime`` returned by 5.0+ servers for the first find/aggregate/distinct operation in a
+   property ``snapshotTimestamp`` of the ``ClientSession`` object. Drivers MUST save the ``atClusterTime``


The name snapshotTimestamp is inconsistent with the other "time" properties, clusterTime, and operationTime. Can we rename it to snapshotTime?

Good point. snapshotTimestamp is not part of public API now, and in fact in C# it's called snapshotTime internally for this consistency reason.
@ajdavis do you have any objections to snapshotTime name?

Hmm okay. This line gave me the impression that the snapshotTimestamp needs to be a public session property:

property snapshotTimestamp of the ClientSession object

Can you clarify that it must be private? Perhaps just saying "...private snapshotTime property..."

I still think it's more consistent to name it "snapshotTime" though.

benjirewis

LGTM given new changes to causal-consistency. Prose test looks good in libmongoc; just had a question about the set server parameter.

benjirewis · 2021-06-24T19:29:41Z

source/sessions/tests/README.rst

+======================
+Snapshot sessions tests require server of version 5.0 or higher and 
+replica set or a sharded cluster deployment.
+The server ``minSnapshotHistoryWindowInSeconds`` parameter SHOULD be configured to match the test execution time.


Does this need to be set? Why is this important for the new tests, and by "test execution time", do you mean how long it takes the spec + prose tests to run?

We want to ensure that the snapshot the test reads still exists on the server.
IIRC the default history is 10 seconds, which was not sufficient at least for .Net tests.
So drivers should be aware about this parameter and set it as needed.

Why was 10 seconds not long enough? Are there snapshot reads tests that take longer than 10 seconds?

It was not sufficient when running in Debug mode.
Apparently it's 5 seconds and .Net tests in release are much quicker. So no changes to EG needed (at least in .Net)

I'd still suggest to track this parameter for such slower dubug mode runs. Not sure what's the best wording though, maybe:

The server minSnapshotHistoryWindowInSeconds parameter MAY be configured to match the tests execution time.

?

Perhaps something like:
"Note that by default the server only keeps the snapshot alive for 10 seconds. Debugging a driver in the middle of a snapshot session may lead to SnapshotTooOld errors. Drivers can work around this issue by increasing the server's minSnapshotHistoryWindowInSeconds parameter, for example:

client.admin.command('setParameter', 1, minSnapshotHistoryWindowInSeconds=60)

ShaneHarvey · 2021-06-24T19:49:16Z

source/sessions/snapshot-sessions.rst

+snapshot reads are implemented by:
+
+1. Saving the ``atClusterTime`` returned by 5.0+ servers for the first find/aggregate/distinct operation in a
+   property ``snapshotTimestamp`` of the ``ClientSession`` object. Drivers MUST save the ``atClusterTime``


Hmm okay. This line gave me the impression that the snapshotTimestamp needs to be a public session property:

property snapshotTimestamp of the ClientSession object

Can you clarify that it must be private? Perhaps just saying "...private snapshotTime property..."

I still think it's more consistent to name it "snapshotTime" though.

ShaneHarvey · 2021-06-24T19:57:52Z

source/sessions/snapshot-sessions.rst

+
+.. code:: typescript
+
+    options = new SessionOptions(isSnapshot = true);


Can we rename isSnapshot to snapshot? Including is in the name doesn't add any meaning, it's just a filler word. It's also inconsistent with other boolean options in the driver api. For example causalConsistency isn't called isCausalConsistency or isCausallyConsistent.

Good point re. consistency. Renamed.

ShaneHarvey · 2021-06-24T20:55:00Z

source/sessions/tests/unified/snapshot-sessions.json

@@ -0,0 +1,706 @@
+{


FYI we usually develop (and review) the yaml version because it's easier to read and supports comments and anchors to reduce the verbosity. Also the json file is auto-generated from the yaml.

ShaneHarvey · 2021-06-24T21:04:54Z

source/sessions/tests/README.rst

+Default snapshot history window on the server is 5 seconds. Running the test in debug mode, or in any other slow configuration
+may lead to `SnapshotTooOld` errors. Drivers can work around this issue by increasing the server's `minSnapshotHistoryWindowInSeconds` parameter, for example:
+
+.. code:: typescript


Should be "python".

ShaneHarvey · 2021-06-24T21:06:47Z

source/sessions/tests/README.rst

+-  Setting both ``snapshot`` and ``causalConsistency`` is not allowed
+
+    * ``client.startSession(snapshot = true, causalConsistency = true)``
+    * Assert that an error was raised by driver


Can we add a unified test for this? It could run the client.startSession operation with snapshot = true, causalConsistency = true arguments and assert that we raise a client-side error with isClientError: true.

That might be a good idea, but session entities creation stage does not have error validation, and we don't have startSession operation. Introducing startSession operation for all drivers instead of prose test seemed overkill in this case.
Is there any simple way to achieve that?

and we don't have startSession operation.

The unified test runner allows us to call any method on the client including startSession. I don't think it's overkill but we don't need to do it in this PR. I'll open a drivers ticket for it.

Edit: or maybe this would require a "schemaVersion" bump in the unified test format. Still I think it's worthwhile to be able to call startSession within tests.

ShaneHarvey · 2021-06-24T21:31:42Z

source/sessions/snapshot-sessions.rst

+2. Passing that ``snapshotTime`` in the ``atClusterTime`` field of the ``readConcern`` field
+   for subsequent snapshot read operations (for find/aggregate/distinct commands).
+
+Server Command Responses


When the driver starts a snapshot session but the server does not return the atClusterTime field the driver needs to raise a client side error indicating that server does not support snapshot reads. If we don't do this then snapshot reads can silently fail.

False alarm. The problem was that my implementation neglected to set readConcern.level to ‘snapshot’. When the driver correctly sends ‘snapshot’ old servers will reject the request with this error:

{'operationTime': Timestamp(1624572085, 12), 'ok': 0.0, 'errmsg': 'read concern level snapshot is only valid in a transaction', 'code': 72, 'codeName': 'InvalidOptions', '$clusterTime': {'clusterTime': Timestamp(1624572085, 12), 'signature': {'hash': b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00', 'keyId': 0}}}

Can we add a unified test case that confirms that the server returns an error when drivers try to use a snapshot session? It should run on 3.6-4.4.

ShaneHarvey · 2021-06-24T22:30:42Z

source/sessions/tests/README.rst

+======================
+Snapshot sessions tests require server of version 5.0 or higher and 
+replica set or a sharded cluster deployment.
+Default snapshot history window on the server is 5 seconds. Running the test in debug mode, or in any other slow configuration


The default was updated to 5 minutes in SERVER-47855 so this section is probably not needed at all anymore.

I think that it's still better to track minSnapshotHistoryWindowInSeconds parameter, for any future updates or longer debugger sessions.

source/sessions/snapshot-sessions.rst

ShaneHarvey

Can you add the .yml versions of the two new tests?:

 source/sessions/tests/unified/snapshot-sessions-not-supported-server-error.yml
 source/sessions/tests/unified/snapshot-sessions.yml

ShaneHarvey

LGTM

BorisDog · 2021-06-26T08:13:10Z

Rebased and merged here.

jmikola · 2021-06-29T20:28:10Z

source/unified-test-format/tests/Makefile

 collection-management: HAS_AJV
 	@ajv test -s $(SCHEMA) -d "../../collection-management/tests/*.yml" --valid

+sessions: HAS_AJV


FYI: "sessions" should also have been added to the all target above. See: #1026 (comment)

- Snapshot session spec and test

d847e2c

BorisDog requested review from ajdavis, dariakp, jyemin and rstam June 16, 2021 10:51

BorisDog commented Jun 16, 2021

View reviewed changes

ajdavis suggested changes Jun 16, 2021

View reviewed changes

jyemin requested a review from benjirewis June 16, 2021 13:46

jyemin assigned BorisDog Jun 16, 2021

- PR comments

7e827b1

BorisDog commented Jun 16, 2021

View reviewed changes

- PR comments

bf0e811

jyemin reviewed Jun 16, 2021

View reviewed changes

source/sessions/snapshot-sessions.rst Show resolved Hide resolved

ajdavis approved these changes Jun 17, 2021

View reviewed changes

alcaeus requested changes Jun 17, 2021

View reviewed changes

- PR comments

617a6af

BorisDog requested a review from alcaeus June 17, 2021 10:57

alcaeus reviewed Jun 17, 2021

View reviewed changes

BorisDog added 4 commits June 17, 2021 17:20

- PR comments

24b994c

- tests validation

d1c4066

- tests validation path update

d6fc58c

- tests validation paths fixes

e02e2a7

jyemin requested changes Jun 17, 2021

View reviewed changes

jyemin reviewed Jun 17, 2021

View reviewed changes

jyemin requested changes Jun 17, 2021

View reviewed changes

BorisDog added 2 commits June 17, 2021 18:03

- PR comments

c27b561

- Tests added and updated

7d753e0

- atClusterTime in Distinct command result documented

BorisDog requested a review from jyemin June 17, 2021 19:49

jyemin requested changes Jun 17, 2021

View reviewed changes

- PR comments

96f4471

- Cleanup

dfb2472

BorisDog requested review from benjirewis and jyemin June 23, 2021 09:09

benjirewis reviewed Jun 23, 2021

View reviewed changes

benjirewis mentioned this pull request Jun 24, 2021

CDRIVER-4034 Allow snapshot reads through sessions mongodb/mongo-c-driver#807

Merged

rstam approved these changes Jun 24, 2021

View reviewed changes

ShaneHarvey reviewed Jun 24, 2021

View reviewed changes

CSHARP-3306: PR comments

694f146

benjirewis approved these changes Jun 24, 2021

View reviewed changes

ShaneHarvey reviewed Jun 24, 2021

View reviewed changes

BorisDog added 2 commits June 24, 2021 23:16

- PR comments

d306a01

- PR comments

2d420d2

ShaneHarvey requested changes Jun 24, 2021

View reviewed changes

ShaneHarvey reviewed Jun 24, 2021

View reviewed changes

ShaneHarvey mentioned this pull request Jun 24, 2021

PYTHON-2767 Support snapshot reads on secondaries mongodb/mongo-python-driver#656

Merged

benjirewis reviewed Jun 25, 2021

View reviewed changes

source/sessions/snapshot-sessions.rst Show resolved Hide resolved

BorisDog added 3 commits June 25, 2021 19:16

- Transactions not allowed with snapshot sessions

9551b41

- Not supported snapshot error validation test for older servers

5fa46e7

- PR comments

ed6639e

BorisDog requested a review from ShaneHarvey June 25, 2021 17:16

ShaneHarvey requested changes Jun 25, 2021

View reviewed changes

- yaml tests introduced

c1739c4

BorisDog requested a review from ShaneHarvey June 25, 2021 21:07

- yaml to json autogen

e1e0672

ShaneHarvey approved these changes Jun 25, 2021

View reviewed changes

BorisDog closed this Jun 26, 2021

jmikola mentioned this pull request Jun 29, 2021

DRIVERS-1816 Test redaction of replies to security sensitive commands #1026

Merged

jmikola reviewed Jun 29, 2021

View reviewed changes

BorisDog deleted the DRIVERS-1607 branch April 1, 2022 15:58

	:Spec Title: Snapshot Reads Specification (See the registry of specs)
	:Spec Title: Snapshot Reads Specification


		Drivers MUST NOT retry errors in SnapshotError category for snapshot reads if ``atClusterTime`` is supplied.

		Test Plan


		.. code:: typescript

		options = new SessionOptions(isSnapshot = true);

DRIVERS-1607: Snapshot reads on Secondaries #1022

DRIVERS-1607: Snapshot reads on Secondaries #1022

Uh oh!

Conversation

BorisDog commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BorisDog commented Jun 16, 2021 •

edited

Loading

benjirewis left a comment •

edited

Loading