RUST-52 Implement Sessions API #304

isabelatkinson · 2021-03-15T14:58:49Z

No description provided.

src/client/options/mod.rs

src/error.rs

src/sdam/description/topology/mod.rs

src/test/spec/sessions/mod.rs

isabelatkinson · 2021-03-15T21:29:01Z

src/test/spec/sessions/mod.rs

+            let result = match operation.object {
+                OperationObject::Collection => {
+                    let result = operation.execute_on_collection(&coll, session).await;
+                    if test.description == "Server supports implicit sessions" {


This one particular test runs two operations with implicit sessions in sequence and then checks to see if they used the same lsid. Delaying here has the same effect as delaying after calling drop (since drop is called when the implicit session goes out of scope); made it conditional to avoid slowing down the tests a ton.

Sounds good, could you add a comment into the code here explaining that?

isabelatkinson · 2021-03-15T21:34:56Z

src/test/spec/sessions/mod.rs

+                }
+                OperationObject::TestRunner => {
+                    match operation.name.as_str() {
+                        "assertDifferentLsidOnLastTwoCommands" => {


All of these operations take in different arguments so it didn't make much sense to integrate them into the usual operation parsing -- I'll abstract this out a little more if it gets too unwieldy when the additional transactions operations are introduced.

isabelatkinson · 2021-03-15T21:36:33Z

src/test/spec/sessions/operation.rs

@@ -0,0 +1,1178 @@
+use std::{collections::HashMap, fmt::Debug, ops::Deref};


This is all quite similar to the operations stuff in the other runners, just some minor adjustments to fit in with the specific test schema for sessions/transactions.

The README for the runner claims it has the same format as the transactions runner, which should mean it has the v2 format (transactions / retryable reads / crud v2). Is there any chance we'll be able to reuse code there instead of introducing an entirely new runner?

isabelatkinson · 2021-03-15T21:38:27Z

src/test/spec/sessions/test_event.rs

+        for (k, v) in &expected.command {
+            if k == "lsid" {
+                match v.as_str().unwrap() {
+                    "session0" => {


The tests specify lsids as the name of the session (e.g.)

src/client/executor.rs

src/client/options/mod.rs

src/coll/mod.rs

src/cursor/session.rs

src/selection_criteria.rs

src/test/spec/sessions/mod.rs

patrickfreed

Looks really good so far! I haven't gotten a chance to review the whole thing yet, but I do have a few minor comments / suggestions on the bits I have seen.

src/coll/mod.rs

src/db/mod.rs

src/sdam/description/topology/mod.rs

patrickfreed · 2021-03-18T01:22:18Z

src/test/spec/sessions/mod.rs

+            let result = match operation.object {
+                OperationObject::Collection => {
+                    let result = operation.execute_on_collection(&coll, session).await;
+                    if test.description == "Server supports implicit sessions" {


Sounds good, could you add a comment into the code here explaining that?

patrickfreed · 2021-03-18T01:32:49Z

src/test/spec/sessions/operation.rs

@@ -0,0 +1,1178 @@
+use std::{collections::HashMap, fmt::Debug, ops::Deref};


The README for the runner claims it has the same format as the transactions runner, which should mean it has the v2 format (transactions / retryable reads / crud v2). Is there any chance we'll be able to reuse code there instead of introducing an entirely new runner?

patrickfreed

Looks great! I just have a few minor comments about the ClientSession type and the tests.

src/client/session/mod.rs

patrickfreed · 2021-03-22T19:48:21Z

src/client/session/mod.rs

+    /// Returns a mutable reference to this session wrapped in an option. This method is necessary
+    /// to bypass the construction of an option taking ownership of a mutable reference.
+    #[allow(clippy::unnecessary_wraps)]
+    pub(crate) fn as_mut_ref_option(&mut self) -> Option<&mut Self> {


I looked into this some more, and the issue has to do with reborrowing (well actually, the fact that a reborrow is not occurring). A reborrow seems to basically be an implicit copy of the reference that the compiler will insert in certain situations such as a function call. What's surprising is that hits behavior works even for mutable references, which do not implement Copy or Clone. It seems that due to the non-lexical-lifetimes feature, the compiler can tell when having multiple mutable references is okay or not. It also seems that the compiler is no longer able to / does not attempt to determine if a reborrow is safe when putting the mutable reference into an enum for passing into execute_operation (I think this is because Option is generic and does not have any &mut specified, see here for an explanation). Calling as_mut_ref_option does work though, since it first reborrows &mut self and then moves that reference into an Option, preventing a use-after-move compiler error. This could similarly be achieved by doing an explicit reborrow like Some(&mut *session). Feel free to keep this method if you think it's clearer or easier to use though.

This is all not documented very well, so most of what I understand of this situation comes from various forums / stackoverflow posts. Check out rust-lang/reference#788 for some useful links (and the request for better documentation on reborrows).

As a funny side note: I introduced MutableSessionRef to actually do the opposite of what our problem here is--I needed a way to force a reference to move rather than reborrow, so I wrapped it in a struct which I could move around.

Huh that's super interesting; I'm surprised there isn't better documentation of this in the Rust book. Updated to use an explicit reborrow.

src/test/spec/v2_runner/test_event.rs

src/test/util/event.rs

src/client/session/mod.rs

src/coll/mod.rs

patrickfreed · 2021-03-24T00:00:11Z

Code changes LGTM, it looks like this just needs to be rebased to fix the merge conflict. I think the compilation issue on 1.45 should go away after that too.

patrickfreed

LGTM! Great job with this!

Side note: there may be some tricky merge conflicts with the error changes in #301, particularly with cursors. It may help to coordinate somehow with @saghm on merging them in.

isabelatkinson · 2021-03-24T17:18:01Z

Side note: there may be some tricky merge conflicts with the error changes in #301, particularly with cursors. It may help to coordinate somehow with @saghm on merging them in.

@saghm I think it would be easiest for you to merge the error changes first; once that's in I can rebase and you can review whatever changes are needed to apply that work to this PR.

saghm

One question/suggestion about making something public and one clarification question about the v2 runner, but other than those, this looks good to me. Great job on this!

saghm · 2021-03-26T20:13:01Z

src/client/session/cluster_time.rs

-pub(crate) struct ClusterTime {
-    cluster_time: Timestamp,
+pub struct ClusterTime {
+    pub cluster_time: Timestamp,


I don't think these should be public; this allows a user who owns a ClusterTime to modify the timestamp or the signature, which is not useful since it would presumably break it. If users do actually need to inspect these values, we should make them private and add getters, but I think that this is supposed to be a mostly opaque value to users (that they receive from the driver and then can pass back in other places without needing to care about the internals), in which case we wouldn't need getters. Since the struct derives Debug, users will still be able to log cluster times even if we don't add getters, so that wouldn't be a reason to add getters either.

That makes sense to me, the reasoning for exposing these was mainly for inspecting, but I think deriving Debug should take care of that.

saghm · 2021-03-26T20:20:02Z

src/test/spec/v2_runner/mod.rs

@@ -0,0 +1,304 @@
+pub mod operation;


This (and the rest of the v2 test runner) is just moved from the previous location in the CRUD spec tests, correct?

yep, I just did some renaming for the sake of clarity

isabelatkinson marked this pull request as draft March 15, 2021 14:59

isabelatkinson commented Mar 15, 2021

View reviewed changes

isabelatkinson requested review from patrickfreed and saghm March 15, 2021 22:41

saghm reviewed Mar 16, 2021

View reviewed changes

isabelatkinson marked this pull request as ready for review March 16, 2021 17:30

patrickfreed reviewed Mar 18, 2021

View reviewed changes

patrickfreed reviewed Mar 22, 2021

View reviewed changes

isabelatkinson force-pushed the RUST-52 branch from b07ee00 to 7385687 Compare March 24, 2021 14:50

patrickfreed approved these changes Mar 24, 2021

View reviewed changes

isabelatkinson force-pushed the RUST-52 branch from fabe75b to 73bd4d9 Compare March 24, 2021 19:00

saghm reviewed Mar 26, 2021

View reviewed changes

saghm approved these changes Mar 29, 2021

View reviewed changes

isabelatkinson added 2 commits March 29, 2021 13:01

RUST-52 Implement Sessions API

a4bcbd9

make ClusterTime members private

95f623e

isabelatkinson force-pushed the RUST-52 branch from bb652a6 to 95f623e Compare March 29, 2021 17:15

isabelatkinson merged commit 9cb95fe into mongodb:master Mar 29, 2021

isabelatkinson deleted the RUST-52 branch March 29, 2021 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-52 Implement Sessions API #304

RUST-52 Implement Sessions API #304

isabelatkinson commented Mar 15, 2021

isabelatkinson Mar 15, 2021

patrickfreed Mar 18, 2021

isabelatkinson Mar 15, 2021

isabelatkinson Mar 15, 2021

patrickfreed Mar 18, 2021

isabelatkinson Mar 15, 2021

patrickfreed left a comment

patrickfreed Mar 18, 2021

patrickfreed Mar 18, 2021

patrickfreed left a comment

patrickfreed Mar 22, 2021

isabelatkinson Mar 23, 2021

patrickfreed commented Mar 24, 2021

patrickfreed left a comment

isabelatkinson commented Mar 24, 2021

saghm left a comment

saghm Mar 26, 2021

isabelatkinson Mar 26, 2021

saghm Mar 26, 2021

isabelatkinson Mar 26, 2021

		@@ -0,0 +1,1178 @@
		use std::{collections::HashMap, fmt::Debug, ops::Deref};

RUST-52 Implement Sessions API #304

RUST-52 Implement Sessions API #304

Conversation

isabelatkinson commented Mar 15, 2021

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

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

patrickfreed commented Mar 24, 2021

patrickfreed left a comment

Choose a reason for hiding this comment

isabelatkinson commented Mar 24, 2021

saghm 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

Choose a reason for hiding this comment