RUST-97 Support sharded transactions recovery token #398

NBSquare · 2021-07-20T14:48:53Z

Implemented recovery tokens from the spec:

Every successful (ok:1) command response in a sharded transaction includes a recoveryToken field. Drivers MUST track the most recently received recoveryToken field and MUST append this field to any subsequent commitTransaction or abortTransaction commands. Tracking the most recently returned recoveryToken allows the server to update the recoveryToken mid-transaction if needed.

This ticket retrieves the recoveryToken from responses and adds it next to the pinned mongos during sharded transactions, passing it back during the appropriate operations.

NBSquare · 2021-07-20T14:55:13Z

src/test/spec/v2_runner/mod.rs

@@ -271,7 +271,8 @@ pub async fn run_v2_test(test_file: TestFile) {
                                .unwrap_or_else(|| panic!("ClientSession is not pinned"));

                            fail_point_guards.push(
-                                internal_client
+                                client
+                                    .deref()


As far as I can tell it doesn't strictly matter here whether the failpoint is enabled on internal_client or the EventClient, but for consistency with the other failpoint we enable I changed this to the EventClient.

Update: the unified test runner specification says not to run failpoint commands on the internal client so we've decided to do the same here.

Hm I agree we should be consistent with which client we're using for failpoints but I think it would make more sense to do all of them with the internal_client rather than the EventClient. In general we just use the EventClient for operations that need to be tracked for command monitoring and the internal client is used for test setup and test runner operations.

NBSquare · 2021-07-20T14:58:05Z

src/test/util/failpoint.rs

@@ -48,10 +48,20 @@ impl FailPoint {
        client: &Client,
        criteria: impl Into<Option<SelectionCriteria>>,
    ) -> Result<FailPointGuard> {
+        // TODO: DRIVERS-1385 remove this logic for moving errorLabels to the top level.


This is necessary because the new spec test (mongos-recovery-token) sets up a failpoint with a RetryableWriteError, which requires errorLabels to be on the failpoint top level when it's initialized. This can also be observed here

NBSquare · 2021-07-20T14:58:59Z

src/test/util/matchable.rs

-            // TODO RUST-97: Remove this logic to bypass recoveryToken
-            if k == "recoveryToken" {
-                continue;
+            if k == "recoveryToken" && v.is_placeholder() {


recoveryToken is tested as follows.

isabelatkinson

looks good! just a few comments/suggestions

isabelatkinson · 2021-07-20T20:17:44Z

src/client/session/mod.rs

 }

 impl Transaction {
    pub(crate) fn start(&mut self, options: Option<TransactionOptions>) {
        self.state = TransactionState::Starting;
        self.options = options;
+        self.recovery_token = None;


Is this assignment necessary? Similarly to pinned_mongos I believe this field should be None by default upon the first transaction and reset to None after previous transactions have finished.

Yeah. From the spec:

Drivers MUST clear a session's cached recoveryToken when transitioning to the "no transaction" or "starting transaction" state.

So, I'm clearing the recoveryToken on Transaction::start and Transaction::reset

src/operation/run_command/mod.rs

isabelatkinson · 2021-07-20T20:42:26Z

src/test/spec/v2_runner/mod.rs

@@ -271,7 +271,8 @@ pub async fn run_v2_test(test_file: TestFile) {
                                .unwrap_or_else(|| panic!("ClientSession is not pinned"));

                            fail_point_guards.push(
-                                internal_client
+                                client
+                                    .deref()


Update: the unified test runner specification says not to run failpoint commands on the internal client so we've decided to do the same here.

Hm I agree we should be consistent with which client we're using for failpoints but I think it would make more sense to do all of them with the internal_client rather than the EventClient. In general we just use the EventClient for operations that need to be tracked for command monitoring and the internal client is used for test setup and test runner operations.

src/test/util/matchable.rs

isabelatkinson

lgtm! tagging in the rest of the team. can you run another evergreen patch and add it to the ticket?

patrickfreed

Looks pretty good! I just have a few minor comments / suggestions

src/test/util/matchable.rs

src/client/executor.rs

src/operation/mod.rs

patrickfreed

LGTM!

NBSquare · 2021-07-27T13:19:14Z

@abr-egn @isabelatkinson please take a look at the last commit when you get the chance - i added a prose test and changed some logic for retrieving the recoveryToken which is probably worth looking at.

NBSquare requested a review from isabelatkinson July 20, 2021 14:48

NBSquare commented Jul 20, 2021

View reviewed changes

isabelatkinson reviewed Jul 20, 2021

View reviewed changes

NBSquare force-pushed the sharded-transactions branch from 349bc6a to 27ec71d Compare July 21, 2021 17:39

NBSquare force-pushed the RUST-97 branch 2 times, most recently from 748f90e to 6f9be83 Compare July 21, 2021 17:43

isabelatkinson approved these changes Jul 21, 2021

View reviewed changes

isabelatkinson requested review from patrickfreed and abr-egn July 21, 2021 22:08

abr-egn approved these changes Jul 22, 2021

View reviewed changes

patrickfreed reviewed Jul 22, 2021

View reviewed changes

src/test/util/matchable.rs Show resolved Hide resolved

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

src/operation/mod.rs Outdated Show resolved Hide resolved

NBSquare force-pushed the sharded-transactions branch from 27ec71d to 6902da1 Compare July 26, 2021 14:52

NBSquare force-pushed the RUST-97 branch from 6f9be83 to f2073b7 Compare July 26, 2021 14:52

NBSquare requested a review from patrickfreed July 26, 2021 14:52

NBSquare force-pushed the RUST-97 branch from f2073b7 to 13512a7 Compare July 26, 2021 15:44

patrickfreed approved these changes Jul 26, 2021

View reviewed changes

NBSquare requested review from abr-egn and isabelatkinson July 27, 2021 13:19

abr-egn approved these changes Jul 27, 2021

View reviewed changes

NBSquare force-pushed the RUST-97 branch from 32d52d3 to fbd8c56 Compare July 27, 2021 14:40

isabelatkinson approved these changes Jul 27, 2021

View reviewed changes

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

75b9253

NBSquare force-pushed the sharded-transactions branch from 6902da1 to b9a7c53 Compare July 27, 2021 17:40

NBSquare force-pushed the RUST-97 branch from fbd8c56 to 744b9b7 Compare July 27, 2021 17:40

NBSquare added 3 commits July 27, 2021 14:15

Add recovery token

fd53d71

Test against recovery token

4274e34

style 💄

6aef003

NBSquare added 2 commits July 27, 2021 14:15

Fix recovery token logic and add test

a4b6ed8

Sanity check that recovery token is sharded

9ee2374

NBSquare force-pushed the RUST-97 branch from 744b9b7 to 9ee2374 Compare July 27, 2021 18:17

Merge branch 'sharded-transactions' into RUST-97

7a71650

NBSquare merged commit ae2b7be into mongodb:sharded-transactions Jul 28, 2021

NBSquare deleted the RUST-97 branch July 28, 2021 13:50

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

RUST-97 Support sharded transactions recovery token (#398)

aba486b

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

RUST-97 Support sharded transactions recovery token (#398)

54dd1e0

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

RUST-97 Support sharded transactions recovery token (#398)

c4f1ada

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

RUST-97 Support sharded transactions recovery token (#398)

b9973a4

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

RUST-97 Support sharded transactions recovery token (#398)

30def14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-97 Support sharded transactions recovery token #398

RUST-97 Support sharded transactions recovery token #398

NBSquare commented Jul 20, 2021

NBSquare Jul 20, 2021

isabelatkinson Jul 20, 2021 •

edited

NBSquare Jul 20, 2021

NBSquare Jul 20, 2021

isabelatkinson left a comment

isabelatkinson Jul 20, 2021

NBSquare Jul 21, 2021

isabelatkinson Jul 20, 2021 •

edited

isabelatkinson left a comment

patrickfreed left a comment

patrickfreed left a comment

NBSquare commented Jul 27, 2021

RUST-97 Support sharded transactions recovery token #398

RUST-97 Support sharded transactions recovery token #398

Conversation

NBSquare commented Jul 20, 2021

NBSquare Jul 20, 2021

Choose a reason for hiding this comment

isabelatkinson Jul 20, 2021 • edited

Choose a reason for hiding this comment

NBSquare Jul 20, 2021

Choose a reason for hiding this comment

NBSquare Jul 20, 2021

Choose a reason for hiding this comment

isabelatkinson left a comment

Choose a reason for hiding this comment

isabelatkinson Jul 20, 2021

Choose a reason for hiding this comment

NBSquare Jul 21, 2021

Choose a reason for hiding this comment

isabelatkinson Jul 20, 2021 • edited

Choose a reason for hiding this comment

isabelatkinson left a comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

NBSquare commented Jul 27, 2021

isabelatkinson Jul 20, 2021 •

edited

isabelatkinson Jul 20, 2021 •

edited