RUST-885 Support snapshot sessions #390

abr-egn · 2021-07-09T19:00:45Z

This adds support for the snapshot session option, and the associated client-side machinery to track read operation session timestamps. This also fixes RUST-878 and RUST-879.

abr-egn · 2021-07-12T13:37:19Z

src/cmap/conn/command.rs

@@ -120,11 +131,21 @@ impl CommandResponse {
        let cluster_time = raw_response
            .get("$clusterTime")
            .and_then(|subdoc| bson::from_bson(subdoc.clone()).ok());
+        let snapshot_time = raw_response


My initial approach was to add atClusterTime as an explicit field in the output of the relevant commands (find, aggregate, distinct), but that required:

introducing a trait for the output type of an operation, which added a lot of churn, and

additional special handling in operations implemented on top of one of those three (e.g. countDocuments).

The approach here avoids both of those issues at the cost of digging a bit into the raw document.

As part of #389, I actually did have to introduce a trait for the output type of an operation (there was definitely a lot of churn, but for that work it was required I think), so we could still go with your original plan once that PR is merged. For the purposes of avoiding merge conflicts, I think it'll probably be easier to merge this PR first though and then rebase #389 off of that and adjust for atClusterTime accordingly there.

Ah, with that trait existing it definitely makes sense to go back to extracting the snapshot time that way. Your plan for merging SGTM.

abr-egn · 2021-07-12T13:38:50Z

src/concern/mod.rs

 #[non_exhaustive]
 pub struct ReadConcern {
    /// The level of the read concern.
    pub level: ReadConcernLevel,
+
+    /// The snapshot read timestamp.
+    #[serde(skip_serializing_if = "Option::is_none")]


The server rejects commands with atClusterTime: null.

Out of curiosity, would adding #[serde(skip_serializing_none)] to the ReadConcern struct do the trick here as well? We use that for a lot of our options structs but I'm wondering if that would break other behavior here.

Ah, I'd missed that, thank you! It looks like that works as well, updated to be consistent with the rest of the codebase.

abr-egn · 2021-07-12T13:40:36Z

src/cursor/common.rs

@@ -152,22 +153,20 @@ pub(crate) struct CursorSpecification {

 impl CursorSpecification {
    pub(crate) fn new(


This refactor was in service of being able to pass an at_cluster_time field through easily, but even without that it seemed worthwhile to leave in.

patrickfreed · 2021-07-12T15:29:41Z

src/error.rs

+    /// The server version does not support the operation.
+    #[error("The server version does not support a database operation: {message}")]
+    #[non_exhaustive]
+    ServerVersion { message: String },


What do you think about naming this IncompatibleServerVersion instead for some added clarity? Or even IncompatibleServer in case there are more criteria in the future unrelated to server version that may cause an operation to fail?

Good call, done.

patrickfreed · 2021-07-12T15:37:17Z

src/cmap/conn/command.rs

@@ -120,11 +131,21 @@ impl CommandResponse {
        let cluster_time = raw_response
            .get("$clusterTime")
            .and_then(|subdoc| bson::from_bson(subdoc.clone()).ok());
+        let snapshot_time = raw_response


As part of #389, I actually did have to introduce a trait for the output type of an operation (there was definitely a lot of churn, but for that work it was required I think), so we could still go with your original plan once that PR is merged. For the purposes of avoiding merge conflicts, I think it'll probably be easier to merge this PR first though and then rebase #389 off of that and adjust for atClusterTime accordingly there.

patrickfreed

LGTM!

isabelatkinson · 2021-07-12T20:47:08Z

src/client/mod.rs

+        self.list_databases_common(filter, options, None).await
+    }
+
+    /// Gets information about each database present in the cluster the Client is connected to.


Can we add "using the provided ClientSession" to this doc (in the same vein as e.g. this)?

Good call, done.

isabelatkinson · 2021-07-12T20:51:10Z

src/concern/mod.rs

 #[non_exhaustive]
 pub struct ReadConcern {
    /// The level of the read concern.
    pub level: ReadConcernLevel,
+
+    /// The snapshot read timestamp.
+    #[serde(skip_serializing_if = "Option::is_none")]


Out of curiosity, would adding #[serde(skip_serializing_none)] to the ReadConcern struct do the trick here as well? We use that for a lot of our options structs but I'm wondering if that would break other behavior here.

abr-egn marked this pull request as draft July 9, 2021 19:03

abr-egn commented Jul 12, 2021

View reviewed changes

abr-egn requested review from isabelatkinson and patrickfreed July 12, 2021 13:51

abr-egn marked this pull request as ready for review July 12, 2021 13:52

patrickfreed reviewed Jul 12, 2021

View reviewed changes

patrickfreed approved these changes Jul 12, 2021

View reviewed changes

isabelatkinson reviewed Jul 12, 2021

View reviewed changes

isabelatkinson approved these changes Jul 13, 2021

View reviewed changes

abr-egn added 19 commits July 13, 2021 16:27

add snapshot option and client state

e68b371

allow atClusterTime in relevant command responses

fec46af

add an OperationResult trait

96f4c27

use ReadConcern struct rather than document construction

bf6acfa

capture timestamp in Distinct result

0519bf3

test fixes

c2a6619

more test fixes

a0f487a

pull in spec test updates

9524df4

support sessions for more unified runner operations

e178ab7

show result mismatch

7c571b1

fix casing of at_cluster_time

e65ecff

print event mismatch

45fb940

directly parse snapshot timestamp from response bson

c46d378

undo OperationResult

5227c3a

update error message

f03d936

remove debug prints

9f2e7cc

use session in runCommand in the test runner

ad2f7ec

clippy and rustfmt fixes

609af01

remove some spurious deltas

c038b60

abr-egn added 3 commits July 13, 2021 16:27

error on snapshot operations for server versions < 5.0

702a463

rename error

31030f7

update comment and serialization

02df866

abr-egn force-pushed the RUST-885/snapshot-session branch from a37d41a to 02df866 Compare July 14, 2021 13:43

abr-egn merged commit 8b808e3 into mongodb:master Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-885 Support snapshot sessions #390

RUST-885 Support snapshot sessions #390

abr-egn commented Jul 9, 2021 •

edited

abr-egn Jul 12, 2021

patrickfreed Jul 12, 2021

abr-egn Jul 12, 2021

abr-egn Jul 12, 2021

isabelatkinson Jul 12, 2021

abr-egn Jul 13, 2021

abr-egn Jul 12, 2021

patrickfreed Jul 12, 2021

abr-egn Jul 12, 2021

patrickfreed Jul 12, 2021

patrickfreed left a comment

isabelatkinson Jul 12, 2021

abr-egn Jul 13, 2021

isabelatkinson Jul 12, 2021

		@@ -152,22 +153,20 @@ pub(crate) struct CursorSpecification {

		impl CursorSpecification {
		pub(crate) fn new(

RUST-885 Support snapshot sessions #390

RUST-885 Support snapshot sessions #390

Conversation

abr-egn commented Jul 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abr-egn commented Jul 9, 2021 •

edited