RUST-122 Support `mongos` pinning for sharded transactions #383

NBSquare · 2021-07-01T18:11:52Z

Implemented mongos pinning on sharded transactions.

Important notes:

Because recovery token hasn't been implemented, there are a few places where the field is expected or a related error is raised. In order to test the desired functionality for this PR, these issues have been suppressed, along with TODO comments for RUST-97 (implement recovery tokens).
src/test/spec/json/transactions has a number of V2 runner tests which are related to pinning, and these are being skipped through a simple check in test::spec::transactions::run. Removing this check would cause recovery token tests to fail. Rather than remove these, I created another method test::spec::transactions::test_sharded_transactions which runs a subset of these V2 tests, as well as the unpin test for the unified runner.

NBSquare · 2021-07-01T19:01:16Z

Removing Patrick and Abraham until Isabel gets a chance to look at this.

src/client/session/mod.rs

src/client/executor.rs

isabelatkinson · 2021-07-06T17:53:05Z

src/client/session/mod.rs

@@ -285,6 +298,9 @@ impl ClientSession {
                }
                .into());
            }
+            TransactionState::Committed { .. } => {


Is it necessary to have this here given that we are unpinning the session at the beginning of execute_operation_with_retry?

I think so. Per the spec, we want to unpin the session when:

A new transaction is started on the ClientSession after the previous transaction has been committed. The session MUST be unpinned before performing server selection for the first operation of the new transaction.

Because start_transaction doesn't execute an operation but changes the transaction state to Starting we want to make sure the session is unpinned, so server selection is performed normally when the first operation on the transaction is executed.

src/client/session/mod.rs

src/sdam/description/topology/mod.rs

durran · 2021-07-09T15:14:31Z

I'm not familiar with the Rust driver's design but just wanted to chime in with a note that could make everyone's lives easier in the future. When implementing load balancer support, the driver will need to be able to pin connections as well, to both cursors and transactions. So may be something to think about now when the pinning related functionality for mongos goes in. Hope that information may help if no one had it yet.

isabelatkinson

one small comment but otherwise this looks great! also it looks like changes from some newer commits on master are being shown in the diff for this PR, can you update the branch to include these commits as well to fix?

isabelatkinson · 2021-07-14T18:48:59Z

src/test/spec/v2_runner/mod.rs

-            None => None,
-        };
+        // TODO RUST-900: Remove this extraneous call.
+        if test.operations.iter().any(|op| op.name == "distinct") {


As a slight optimization, we can also add conditions here only to run distinct on each host when we're working with a 4.2 sharded cluster, as that's the only topology where we run into this issue.

That sounds good. As an aside, some of the other drivers have also run into issues related to StaleDbVersion on other versions. However, it sounds like these bugs are weird and driver-specific.

Since we only ran into the issue on 4.2 sharded, I think it makes sense to add this check, but I want to draw attention to the fact that the issue has impacted other versions on other drivers.

isabelatkinson

Just one question from me, going to tag in the rest of the team for review. Can you run a full evg patch (i.e. all the variants prefixed with !) and add a link to it on the ticket? (Note to reviewers: evergreen patches aren't running automatically because this PR is on a separate branch. Patches are linked in the comments on RUST-122.)

src/test/spec/v2_runner/mod.rs

abr-egn

LGTM!

src/sdam/description/topology/mod.rs

patrickfreed

I have a few minor suggestions, but otherwise looks good!

src/client/executor.rs

src/test/spec/json/sharded-transactions/mongos-pin-auto-tests.py

src/test/spec/unified_runner/test_runner.rs

src/client/executor.rs

patrickfreed

Looks really good, just have one minor suggestion

src/client/executor.rs

patrickfreed

LGTM! great job with this

NBSquare requested review from patrickfreed, isabelatkinson and abr-egn and removed request for patrickfreed and abr-egn July 1, 2021 18:11

isabelatkinson reviewed Jul 6, 2021

View reviewed changes

NBSquare force-pushed the RUST-122 branch from a742b52 to 363d484 Compare July 9, 2021 13:30

NBSquare force-pushed the RUST-122 branch 4 times, most recently from 9351a6d to d369d42 Compare July 13, 2021 20:48

isabelatkinson reviewed Jul 14, 2021

View reviewed changes

NBSquare force-pushed the RUST-122 branch from d369d42 to 6aa8124 Compare July 14, 2021 19:57

NBSquare requested a review from isabelatkinson July 14, 2021 19:57

isabelatkinson reviewed Jul 15, 2021

View reviewed changes

src/test/spec/v2_runner/mod.rs Outdated Show resolved Hide resolved

isabelatkinson requested review from patrickfreed and abr-egn July 15, 2021 14:43

NBSquare force-pushed the RUST-122 branch from 6aa8124 to ee14dcd Compare July 15, 2021 14:53

NBSquare changed the base branch from sharded-transactions to master July 15, 2021 18:23

NBSquare changed the base branch from master to sharded-transactions July 15, 2021 18:23

abr-egn approved these changes Jul 15, 2021

View reviewed changes

src/sdam/description/topology/mod.rs Outdated Show resolved Hide resolved

patrickfreed reviewed Jul 15, 2021

View reviewed changes

NBSquare force-pushed the RUST-122 branch from ee14dcd to 6b06e80 Compare July 16, 2021 15:13

NBSquare requested a review from patrickfreed July 16, 2021 15:14

patrickfreed reviewed Jul 16, 2021

View reviewed changes

src/client/executor.rs Outdated Show resolved Hide resolved

src/client/executor.rs Outdated Show resolved Hide resolved

patrickfreed reviewed Jul 19, 2021

View reviewed changes

src/client/executor.rs Outdated Show resolved Hide resolved

Nathan Blinn added 2 commits July 19, 2021 13:29

Allow executor to pin

b094d30

Enable unpinning sessions

f1756f1

Nathan Blinn added 8 commits July 19, 2021 13:29

Re-add topology check

245260e

Call distinct before v2 tests

8b6667a

Create collection during insertData

3b1bfa6

Use contains_label

5f26d52

Add additional check to distinct

f6ab1d9

Linting and syntax polish 🚨

fdba5d6

Rearrange unpin logic for retry cases & style 💄

cc263ae

Remove spurious serverless attribute

ee967ec

NBSquare force-pushed the RUST-122 branch from 8db966a to b2bd7a7 Compare July 19, 2021 17:30

NBSquare requested review from patrickfreed, abr-egn and isabelatkinson July 19, 2021 17:31

Move some unpin logic behind add_labels

3e6fe5b

NBSquare force-pushed the RUST-122 branch from b2bd7a7 to 3e6fe5b Compare July 19, 2021 18:50

patrickfreed approved these changes Jul 19, 2021

View reviewed changes

isabelatkinson approved these changes Jul 19, 2021

View reviewed changes

abr-egn approved these changes Jul 19, 2021

View reviewed changes

NBSquare merged commit 349bc6a into mongodb:sharded-transactions Jul 19, 2021

NBSquare deleted the RUST-122 branch July 19, 2021 20:56

NBSquare added a commit that referenced this pull request Jul 21, 2021

RUST-122 Support mongos pinning for sharded transactions (#383)

27ec71d

NBSquare added a commit to NBSquare/mongo-rust-driver that referenced this pull request Jul 21, 2021

RUST-122 Support mongos pinning for sharded transactions (mongodb#383)

77f87a6

NBSquare added a commit that referenced this pull request Jul 26, 2021

RUST-122 Support mongos pinning for sharded transactions (#383)

6902da1

NBSquare added a commit to NBSquare/mongo-rust-driver that referenced this pull request Jul 26, 2021

RUST-122 Support mongos pinning for sharded transactions (mongodb#383)

eb98c8d

NBSquare added a commit that referenced this pull request Jul 27, 2021

RUST-122 Support mongos pinning for sharded transactions (#383)

b9a7c53

NBSquare added a commit to NBSquare/mongo-rust-driver that referenced this pull request Jul 27, 2021

RUST-122 Support mongos pinning for sharded transactions (mongodb#383)

75b9253

NBSquare added a commit that referenced this pull request Jul 29, 2021

RUST-122 Support mongos pinning for sharded transactions (#383)

3b03304

NBSquare added a commit that referenced this pull request Jul 30, 2021

RUST-122 Support mongos pinning for sharded transactions (#383)

96fc890

NBSquare added a commit that referenced this pull request Aug 2, 2021

RUST-122 Support mongos pinning for sharded transactions (#383)

137ee60

NBSquare added a commit that referenced this pull request Aug 2, 2021

RUST-122 Support mongos pinning for sharded transactions (#383)

112e9de

NBSquare added a commit that referenced this pull request Aug 2, 2021

RUST-122 Support mongos pinning for sharded transactions (#383)

f77590c

RUST-122 Support mongos pinning for sharded transactions #383

RUST-122 Support mongos pinning for sharded transactions #383

Uh oh!

Conversation

NBSquare commented Jul 1, 2021

Uh oh!

NBSquare commented Jul 1, 2021

Uh oh!

Uh oh!

Uh oh!

isabelatkinson Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

NBSquare Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

durran commented Jul 9, 2021

Uh oh!

isabelatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

isabelatkinson Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

NBSquare Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

isabelatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abr-egn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrickfreed left a comment

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!

patrickfreed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrickfreed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RUST-122 Support `mongos` pinning for sharded transactions #383

RUST-122 Support `mongos` pinning for sharded transactions #383

NBSquare Jul 7, 2021 •

edited

Loading