PHPLIB-466: Support mongos pinning for sharded transactions #677

alcaeus · 2019-09-02T18:10:34Z

https://jira.mongodb.org/browse/PHPLIB-466

alcaeus · 2019-09-02T18:14:42Z

tests/SpecTests/RetryableWritesSpecTest.php

@@ -29,15 +28,13 @@ public function testRetryableWrites(stdClass $test, array $runOn = null, array $
            $this->markTestSkipped('Transaction numbers are only allowed on a replica set member or mongos (PHPC-1415)');
        }

-        if (isset($test->useMultipleMongoses) && $test->useMultipleMongoses && $this->isShardedCluster()) {
-            $this->manager = new Manager(static::getUri(true));


I realised way too late that we don't use $this->manager in these tests. This was exposed when testing session pinning, as running with a single mongos won't trigger pinning logic in libmongoc.

Had we used $this->manager, which is set from MongoDB\Tests\FunctionalTestCase::setUp(), I don't think that would have worked either. That manager is constructed purely from the URI environment variable and wouldn't have considered the spec tests' useMultipleMongoses option.

That's why I originally added this conditional to recreate the manager with multiple mongooses if specified in the spec test, not realising that it still didn't have any effect.

alcaeus · 2019-09-02T18:18:54Z

tests/SpecTests/TransactionsSpecTest.php

@@ -36,6 +37,9 @@ class TransactionsSpecTest extends FunctionalTestCase
    private static $incompleteTests = [
        'error-labels: add unknown commit label to MaxTimeMSExpired' => 'PHPC-1382',
        'error-labels: add unknown commit label to writeConcernError MaxTimeMSExpired' => 'PHPC-1382',
+        'mongos-recovery-token: commitTransaction retry fails on new mongos' => 'isMaster failpoints cannot be disabled',


I was not able to reliably clear the failCommand fail point if it's configured to fail a certain amounts of times. It seems that we don't trigger the fail point the expected number of times, which causes issues when removing the fail point (as isMaster will cause an error). However, even trying to remove the fail point in a loop did not always clear the fail point, which causes failures in following tests.

Reading the script for this test and the comments therein, I wonder if it was written with multi-threaded SDAM in mind. I wasn't aware we had any tests failing isMaster commands before reading this. It looks like this test dates back to the original spec change for recovery tokens in SPEC-1168.

Perhaps you can reach out to @bazile-clyde to see whether the C driver is able to test this with a multi- or single-threaded client. Multi-threaded clients would offload all isMaster activity to a separate background thread and socket, which means these isMaster failures might not affect the client under test at all.

Ah, the comments aren't present in the JSON files, which is why I missed the detailed explanation. The test is implemented in the C driver, but I couldn't find out whether it uses a single- or multi-threaded client. I'll check with Clyde on this.

All of our fail points are activated using single-threaded clients. Also, to disable the fail point we didn't use any loops but instead just sent the command {'configureFailPoint': 'failCommand' 'mode': 'off'}. Both activating and deactivating fail points were done via a new client, pinned to the same server ID as the one we used for testing.

alcaeus · 2019-09-02T18:19:54Z

tests/SpecTests/TransactionsSpecTest.php

    {
-        parent::setUpBeforeClass();
+        parent::setUp();


This change makes the test more compliant with the spec, which requires that all sessions be killed before running a test. We still kill all sessions after a failing test.

src/Client.php

jmikola · 2019-09-04T01:47:16Z

src/Operation/Watch.php

@@ -376,7 +378,7 @@ private function resume($resumeToken = null, $hasAdvanced = false)
        $this->hasResumed = true;

        // Select a new server using the original read preference
-        $server = $this->manager->selectServer($this->aggregateOptions['readPreference']);
+        $server = select_server($this->manager, $this->aggregateOptions['readPreference'], extract_session_from_options($this->aggregateOptions));


I don't think this change is necessary. Quoting myself from PHPLIB-466:

Watch also invokes selectServer() when resuming, but I think that should be left as-is. AFAIK, change streams and transactions are mutually exclusive, but you may want to double-check that just to be sure.

A watch operation is currently not allowed in a transaction:

MongoDB\Driver\Exception\CommandException : Operation not permitted in transaction :: caused by :: Operation not permitted in transaction :: caused by :: $changeStream cannot run with a readConcern other than 'majority', or in a multi-document transaction. Current readConcern: { readConcern: { level: "snapshot" } }

I originally changed all instances of server selection within the library with the new logic so that it's consistent. In this case, changing server selection back would mean that running the watch operation on a pinned session will error because we're running the command on the wrong server, instead of informing the user that the operation can't be done in a transaction. Even in this technically impossible case, I'd still pick the pinned server to have the correct exception shown to the user.

This will also avoid problems down the line if watch ever becomes available in a transaction.

I concur that this would ensure the user gets more accurate error message. Would you mind adding a comment to document this reasoning?

src/functions.php

tests/FunctionalTestCase.php

tests/SpecTests/Operation.php

tests/SpecTests/TransactionsSpecTest.php

jmikola · 2019-09-04T02:29:03Z

tests/SpecTests/RetryableWritesSpecTest.php

@@ -29,15 +28,13 @@ public function testRetryableWrites(stdClass $test, array $runOn = null, array $
            $this->markTestSkipped('Transaction numbers are only allowed on a replica set member or mongos (PHPC-1415)');
        }

-        if (isset($test->useMultipleMongoses) && $test->useMultipleMongoses && $this->isShardedCluster()) {
-            $this->manager = new Manager(static::getUri(true));


Had we used $this->manager, which is set from MongoDB\Tests\FunctionalTestCase::setUp(), I don't think that would have worked either. That manager is constructed purely from the URI environment variable and wouldn't have considered the spec tests' useMultipleMongoses option.

alcaeus · 2019-09-04T11:13:13Z

tests/FunctionalTestCase.php

        if ($this->isShardedCluster()) {
-            $this->markTestSkipped('Transactions are not supported on sharded clusters');
+            if (!$this->isShardedClusterUsingReplicasets()) {


We can't easily figure out what storage engine is in use on sharded clusters, so we're skipping that check when running a sharded setup. I don't think we even run any tests on the MMAPv1 storage engine at all in PHPLIB. This may change once we move library tests to Evergreen, but I'd revisit this then and figure out a more robust solution.

MMAPv1 was completely removed in 4.2, so this shouldn't be a concern.

alcaeus · 2019-09-04T12:42:56Z

@jmikola I've added the prose tests for mongos pinning. I'm not sure if they provide additional value to the spec tests (especially since they cover a range of pin/unpin scenarios), but I've added them nonetheless 👍

jmikola · 2019-09-04T14:26:30Z

tests/SpecTests/TransactionsSpecTest.php

+     */
+    public function testStartingNewTransactionOnPinnedSessionUnpinsSession()
+    {
+        $this->skipIfTransactionsAreNotSupported();


Perhaps we should just move this to setUp().

jmikola · 2019-09-04T14:33:50Z

tests/SpecTests/TransactionsSpecTest.php

+     * ClientSession unpins the session and normal server selection is performed
+     * for the next operation.
+     */
+    public function testStartingNewTransactionOnPinnedSessionUnpinsSession()


FWIW, I opened SPEC-1433 to determine if these prose tests are still relevant. I see no harm in adding them, though.

The isMaster failpoint can't be reliably disabled again in these tests, causing subsequent test failures.

alcaeus requested a review from jmikola September 2, 2019 18:10

alcaeus self-assigned this Sep 2, 2019

alcaeus force-pushed the phplib-466 branch from 9f5e6bd to 29e71c9 Compare September 2, 2019 18:20

alcaeus commented Sep 2, 2019

View reviewed changes

alcaeus force-pushed the phplib-466 branch 2 times, most recently from ebd3e95 to 5a9872a Compare September 3, 2019 05:21

jmikola reviewed Sep 4, 2019

View reviewed changes

alcaeus force-pushed the phplib-466 branch from 5a9872a to 068a32e Compare September 4, 2019 11:08

alcaeus commented Sep 4, 2019

View reviewed changes

alcaeus force-pushed the phplib-466 branch from 068a32e to ecb5538 Compare September 4, 2019 11:15

PHPLIB-466: Require RC1 of the MongoDB driver

4a3db49

alcaeus force-pushed the phplib-466 branch from 5f6af21 to f970795 Compare September 4, 2019 12:37

jmikola reviewed Sep 4, 2019

View reviewed changes

jmikola approved these changes Sep 4, 2019

View reviewed changes

alcaeus added 6 commits September 4, 2019 17:24

PHPLIB-466: Respect pinned session during server selection

9d0cbbb

PHPLIB-466: Allow transactions to run on sharded clusters in MongoDB 4.2

1245eb9

PHPLIB-466: Fix usage of useMultipleMongoses in spec tests

96311aa

PHPLIB-466: Skip tests creating isMaster failpoints

97d486e

The isMaster failpoint can't be reliably disabled again in these tests, causing subsequent test failures.

PHPLIB-466: Improve transaction detection logic

e774434

PHPLIB-466: Implement mongos pinning prose tests

9281873

alcaeus force-pushed the phplib-466 branch from f970795 to 9281873 Compare September 4, 2019 15:25

alcaeus added a commit that referenced this pull request Sep 4, 2019

Merge pull request #677

6a86013

alcaeus merged commit 9281873 into mongodb:master Sep 4, 2019

alcaeus deleted the phplib-466 branch September 4, 2019 16:03

PHPLIB-466: Support mongos pinning for sharded transactions #677

PHPLIB-466: Support mongos pinning for sharded transactions #677

Uh oh!

Conversation

alcaeus commented Sep 2, 2019

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!

jmikola Sep 4, 2019 • 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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

alcaeus commented Sep 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola Sep 4, 2019 •

edited

Loading