PERL-875 - Transaction Support #163

TBSliver · 2018-06-14T15:34:21Z

First pass of Transaction Support for MongoDB 4.0

Currently the test (t/transactions-spec.t) has several TODO parts in the test - the majority of these require PERL-918 ticket to be completed, however they should all start working after that point.

The run-command test run command fails with explicit secondary read preference also has a TODO on it. The test yml looks like this:

  - description: run command fails with explicit secondary read preference

    operations:
      - name: startTransaction
        object: session0
      - name: runCommand
        object: database
        command_name: find
        arguments:
          session: session0
          command:
            find: *collection_name
          readPreference:
            mode: Secondary
        result:
          errorContains: read preference in a transaction must be primary

the issue here is, the spec specifically states The transaction's read preference MUST override all other user configurable read preferences. This is obvious for a client, database, or collection level read preference configuration (and is how it is normally inherited), and is arguable that an explicit run_command with a read preference would also count as user configurable. As well as that, there is no way to determine at the op level, without adding a large amount of extra logic, that the read preference was set explicitly instead of inherited - and this seems to also be the only case or way for a read preference to be set outside of a normal config option.

Either way this is now passed forward for initial code review, pending any changes to the spec in the meantime.

xdg

I haven't gone in detail through the test file, but there's enough for you to start working on changes while I get to that. :-)

Also, please note that server selection is suppose to attach TransientTransactionError or UnknownTransactionCommitResult labels, so you'll need to wrap selection and have some logic to possibly put one or the other error on. It's a little janky, but I think you could toggle the label type depending on the "force" option to send_retryable -- as only commit and abort get forced so those you can set the UnknownTransactionCommitResult label (and then abort throws away any errors).

xdg · 2018-06-14T22:04:47Z

lib/MongoDB/ClientSession.pm

+        # applied after to not override the clone with the original
+        defaultTransactionOptions => {
+          defined( $_[0] )
+            && ref( $_[0] ) eq 'HASH'


These guards are necessary for line 96, too, so maybe move up for the whole sub?

Ah oops thought isa fired before coerce, knew it was one way or the other... heh

xdg · 2018-06-14T22:07:02Z

lib/MongoDB/ClientSession.pm

-        %{ $_[0] }
+        %{ $_[0] },
+        # applied after to not override the clone with the original
+        defaultTransactionOptions => {


I think this would be clearer if you cloned the default transaction options in a separate operation rather than nesting it in with the other cloning.

Right, have done that before building this hash, along with sorting out the guards mentioned below

xdg · 2018-06-14T22:07:46Z

lib/MongoDB/ClientSession.pm

+has _current_transaction_settings => (
+    is => 'rwp',
+    isa => HashRef,
+    init_arg => undef,


needs 'init_arg'?

True, as well as the clearer which I think I was using earlier but became redundant because we just set it completely later.

xdg · 2018-06-14T22:08:30Z

lib/MongoDB/ClientSession.pm

+);
+
+# Flag used to say we are still in a transaction
+has _active_transaction => (


Is this not redundant with _transaction_state? Can it just be a method that computes a boolean based on _transaction_state?

The problem is that theres no easy logic for whether we are in an active transaction - the state must be changed for committed or aborted before the command is sent (so that on error we are in the correct state), but if we are in one of those states and not in a transaction we need to reset the session state back to none - which would mean needing a flag notifying if we are in an actual commit or abort command - and the active transaction flag seems a lot more useful and can be set correctly in the right spots. Also means that setting the transaction state back to 'none' is an easy logic check!

Ok. Keep it, but I'll look closer after the GA release and see what can be done about it.

xdg · 2018-06-14T22:10:39Z

lib/MongoDB/ClientSession.pm

+
+# Flag used to say whether any operations have been performed on the
+# transaction
+has _has_transaction_operations => (


is this not also something we can derive from _transaction_state?

Not fully - the biggest problem is being able to call commit multiple times. Its easy if you are coming from starting or in_progress states, but after that you are in the committed state - at which point, its unknown what state you were in before. I thought about tracking previous state as well, but if you then call commit more than twice (which is allowed AFAICT) it would break - so find just setting a boolean more useful.

xdg · 2018-06-15T19:09:45Z

lib/MongoDB/ClientSession.pm

+            UnknownReplWriteConcern
+            NoSuchTransaction
+        / ) {
+            push @{ $err->error_labels }, 'UnknownTransactionCommitResult';


This label also needs to be applied to server selection errors, network errors, network timeout errors, and retryable write errors.

Yep, those are the only occurances for it not being applied - in the latest version have had to put int he actual error code numbers in as some of the spec tests dont actually set the codeName... (error-labels.json in the last two omit tests)

xdg · 2018-06-15T19:11:19Z

t/transactions-spec.t

+my $server_version = server_version($conn);
+my $server_type    = server_type($conn);
+
+plan skip_all => "Requires MongoDB 4.0"


For the 3.6 tests, I added a test helper function for "skip_unless_sessions". Please make a similar one for "skip_unless _transactions" here.

xdg · 2018-06-15T19:12:26Z

t/transactions-spec.t

+);
+
+my $dir      = path("t/data/transactions");
+my $iterator = $dir->iterator; my $index = 0; # TBSLIVER


Don't forget to remove this

xdg · 2018-06-15T19:12:42Z

t/transactions-spec.t

+
+    subtest $path => sub {
+
+        for my $test ( @{ $plan->{tests} } ) { # TBSLIVER run specific subtest


xdg · 2018-06-15T19:14:48Z

t/transactions-spec.t

+                # a seperate step.
+                $test_db->run_command([ create => $test_coll_name ]);
+
+                my $test_coll = $test_db->get_collection(


Redundant. Just save the first one at line 121 before you call 'drop' on it.

xdg

Looks really close. Please make these changes and then get started on rebasing and integrating with PERL-918 work.

xdg · 2018-06-21T15:32:18Z

lib/MongoDB/ClientSession.pm

-      };
+        # Will cause the isa requirement to fire
+        return unless defined( $_[0] ) && ref( $_[0] ) eq 'HASH';
+        my $dto = $_[0]->{defaultTransactionOptions};


Should this be making a clone as well?

xdg · 2018-06-21T15:34:54Z

lib/MongoDB/ClientSession.pm

+        $dto ||= {};
+        $_[0] = {
+            causalConsistency => 1,
+            %{ $_[0] },


I find this confusing because there are only two available options right now and both are also listed in the clone. Maybe just assign directly to the copies?
{ causalConsistency => $_[0]->{causalConsistency}, defaultTransactionOptions => { %{$_[0]->{defaultTransactionOptions} || {} }}

mm... premature optimisation I think I did there, had mis-remembered the session spec whcih just says 'in a backwards compatible way' for storing sessionOptions. Have changed it to direct assignment anyway.

xdg · 2018-06-21T15:41:09Z

lib/MongoDB/MongoClient.pm

+
    # increment transaction id before write, but otherwise is the same for both attempts
-    $op->session->_server_session->_increment_transaction_id;
+    $op->session->_increment_transaction_id;


xdg · 2018-06-21T15:43:47Z

lib/MongoDB/_TransactionOptions.pm

+);
+use namespace::clean -except => 'meta';
+
+# Options provided during strart transaction


coveralls · 2018-06-22T11:56:52Z

Coverage decreased (-1.7%) to 75.246% when pulling 086d2e0 on shadow-dot-cat:TBSliver/PERL-875 into 9687f61 on mongodb:master.

transactions

one requires PERL-918 to be finished, the other needs some thought

…found

…a transaction

TBSliver · 2018-06-22T12:53:40Z

that should be everything, Have had to update the dependency on Safe::Isa for the $_call_if_can usage - although that could be downgraded again if that code is modified to not use that function.

xdg · 2018-06-22T20:47:09Z

Rebased, squashed and merged as 9162d1d

xdg suggested changes Jun 15, 2018

View reviewed changes

xdg suggested changes Jun 21, 2018

View reviewed changes

TBSliver force-pushed the TBSliver/PERL-875 branch from 3e9d68a to 075d97f Compare June 21, 2018 17:39

TBSliver added 26 commits June 22, 2018 13:13

PERL-918 updated retryable-writes test specs

b2c8df1

minor: Added Callback class to factor out basic monitoring callback work

52c9ac5

PERL-918 - refactored spec tests for retryable writes

2fea40f

PERL-918 Add new error names and codes

4d2d53a

PERL-918 Finish updating tests for retryable-writes

ce08190

PERL-918 re-arrange retry error logic and add clearer comments

1dc7d81

PERL-875 first pass of transactions in sessions

c08d09d

PERL-875 fix minor issues introduced in ClientSession and Types for

0feeebb

transactions

PERL-875 Added boilerplate for PERL-875 spec tests

3423d02

PERL-875 Move transaction spec test boilerplate to normal t directory

aa9fe53

PERL-875 iterate over test files

f7ad672

PERL-875 more implementation work for Transactions and errors

4c20e5b

PERL-875 Added transaction test spec files

129f406

PERL-875 added wip transactions-spec test

06697c2

PERL-875 Fix issue caused in ErrorThrower

11bf6b4

PERL-875 working through spec tests

48ea0f8

PERL-875 adding unknown transaction commit result errorlabel

f5624d5

PERL-875 finishing off spec tests. 2 TODO's left in the tests

02e5bdb

one requires PERL-918 to be finished, the other needs some thought

PERL-875 remove unnecessary flag setting and redundant comments

9056a3f

PERL-875 Refactor transaction options into seperate class

3af32c8

PERL-875 rework session option merging

cabe081

PERL-875 remove TransactionError class and use UsageError class

1fe1a51

PERL-875 refactor write_op and retryable_write_op to be easier to read

70d09a6

PERL-875 make last_error_labels return empty array if no errorLabels …

24f81ed

…found

PERL-875 Factor out session state reset

4f97148

PERL-875 Comment on why needing rw on read_preference

6f74c71

TBSliver added 11 commits June 22, 2018 13:13

PERL-875 resync spec tests - error-labels test still requires PERL-918

87f1e96

PERL-875 Update transaction-spec test, removing 918 todos

bd12150

PERL-875 Move session state change to MongoClient

cdb3066

PERL-918 Build client from clientOptions in test spec

6024c11

PERL-875 rearrange readConcern modification and state setting during …

64283b7

…a transaction

PERL-875 supress count deprecation warning specifically for one test

8bf6d73

PERL-875 Move error label application into session code

906c128

PERL-875 Change option setting to direct assignment

7c0fb28

PERL-875 Change to using constants for transaction state tracking

23d9eee

PERL-875 Fix typo in comment

edb44fa

PERL-875 Ignore all errors from abortTransaction network calls

d8bd31f

TBSliver force-pushed the TBSliver/PERL-875 branch from 2702d1f to d8bd31f Compare June 22, 2018 12:31

PERL-875 $_call_if_can requires Safe::Isa >= 1.000007

086d2e0

xdg mentioned this pull request Jun 22, 2018

PERL-918 Update retryable writes spec #165

Closed

xdg closed this Jun 22, 2018

TBSliver deleted the TBSliver/PERL-875 branch September 14, 2018 14:59


		subtest $path => sub {

		for my $test ( @{ $plan->{tests} } ) { # TBSLIVER run specific subtest

PERL-875 - Transaction Support #163

PERL-875 - Transaction Support #163

Uh oh!

Conversation

TBSliver commented Jun 14, 2018

Uh oh!

xdg left a comment

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!

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!

xdg left a comment

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!

coveralls commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TBSliver commented Jun 22, 2018

Uh oh!

xdg commented Jun 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jun 22, 2018 •

edited

Loading