Propagate the original error for write errors labeled `NoWritesPerformed` #1013

stIncMale · 2022-10-08T00:33:36Z

TODO for @stIncMale: what's achieved by introducing NoWritesPerformed given that we already have RetryableWriteError?

JAVA-4701

driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java

…med` JAVA-4701

jyemin · 2022-10-08T13:52:45Z

@stIncMale have a look at https://jira.mongodb.org/browse/SERVER-66116 for the original report that led to this.

jyemin

Commented

JAVA-4701

stIncMale · 2022-10-12T14:17:01Z

...c/test/functional/com/mongodb/internal/operation/MixedBulkWriteOperationSpecification.groovy

        def operation = new MixedBulkWriteOperation(getNamespace(),
-                [new InsertRequest(new BsonDocument('_id', new BsonInt32(7))),
-                 new InsertRequest(new BsonDocument('_id', new BsonInt32(1)))   // duplicate key
-                ], false, ACKNOWLEDGED, true)
+                [new DeleteRequest(new BsonDocument('_id', new BsonInt32(2))), // existing key
+                 new InsertRequest(new BsonDocument('_id', new BsonInt32(1)))  // existing (duplicate) key
+                ], true, ACKNOWLEDGED, true)

        def failPoint = BsonDocument.parse('''{
            "configureFailPoint": "failCommand",
            "mode": {"times": 2 },
-            "data": { "failCommands": ["insert"],
+            "data": { "failCommands": ["delete"],
                      "writeConcernError": {"code": 91, "errmsg": "Replication is being shut down"}}}''')
        configureFailPoint(failPoint)


I don't have a complete understanding of these test, but here is what I think I understand:

it needs to fail the first attempt with a write concern error, and then it needs to see that the it is retried;

to achieve the above, it fails the second attempt with a different error (tries to insert a duplicate key), and expects to observe it.

The latest server labels the second error as NoWritesPerformed, because it fails to insert a duplicate key. As a result, the updated driver chooses the first error, thus not letting the test to observe the error from the second attempt, i.e., not letting the test to see that the command was retried. By deleting one of the existing keys, the test prevents the server from reporting NoWritesPerformed, which leads to the test being able to observe the error from the second attempt.

Ack, though I'm not sure we're testing the same thing anymore.

stIncMale · 2022-10-12T14:19:41Z

It turned out that the fail points from the new nests did not leak into the failing tests, which means that the changes to Fixture and FailPoint are not necessary. However, these changes do prevent leaking fail points in some cases, which is why I think they should be committed. If you think they should be moved to a different PR, please let me know.

…API.

jyemin · 2022-10-12T18:19:15Z

...c/test/functional/com/mongodb/internal/operation/MixedBulkWriteOperationSpecification.groovy

        def operation = new MixedBulkWriteOperation(getNamespace(),
-                [new InsertRequest(new BsonDocument('_id', new BsonInt32(7))),
-                 new InsertRequest(new BsonDocument('_id', new BsonInt32(1)))   // duplicate key
-                ], false, ACKNOWLEDGED, true)
+                [new DeleteRequest(new BsonDocument('_id', new BsonInt32(2))), // existing key
+                 new InsertRequest(new BsonDocument('_id', new BsonInt32(1)))  // existing (duplicate) key
+                ], true, ACKNOWLEDGED, true)

        def failPoint = BsonDocument.parse('''{
            "configureFailPoint": "failCommand",
            "mode": {"times": 2 },
-            "data": { "failCommands": ["insert"],
+            "data": { "failCommands": ["delete"],
                      "writeConcernError": {"code": 91, "errmsg": "Replication is being shut down"}}}''')
        configureFailPoint(failPoint)


Ack, though I'm not sure we're testing the same thing anymore.

driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java

driver-sync/src/test/functional/com/mongodb/client/Fixture.java

jyemin · 2022-10-12T19:28:24Z

It turned out that the fail points from the new nests did not leak into the failing tests, which means that the changes to Fixture and FailPoint are not necessary. However, these changes do prevent leaking fail points in some cases, which is why I think they should be committed. If you think they should be moved to a different PR, please let me know.

I only noticed this comment after posting mine. I don't mind the change to Failpoint, but I don't see the need for the change to Fixture.

jyemin

LGTM

stIncMale requested a review from jyemin October 8, 2022 00:33

stIncMale commented Oct 8, 2022

View reviewed changes

driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java Outdated Show resolved Hide resolved

Propagate the original error for write errors labeled `NoWritesPerfor…

cf3b483

…med` JAVA-4701

stIncMale force-pushed the JAVA-4701 branch from d523fcc to cf3b483 Compare October 8, 2022 01:00

jyemin requested changes Oct 8, 2022

View reviewed changes

jyemin assigned stIncMale Oct 10, 2022

stIncMale added 3 commits October 11, 2022 11:38

Fix the first fail point in the test

f9759ae

JAVA-4701

Fix the first fail point in the test and document some error labels

7cc1aae

JAVA-4701

Is this the fix?

612b7e7

stIncMale commented Oct 12, 2022

View reviewed changes

Undo javadoc changes because error labels are not part of the public …

5c4deda

…API.

stIncMale requested a review from jyemin October 12, 2022 14:25

jyemin requested changes Oct 12, 2022

View reviewed changes

stIncMale added 2 commits October 12, 2022 16:18

Address review concerns

224315e

Fix javadocs link

58438ff

stIncMale requested a review from jyemin October 14, 2022 20:29

jyemin approved these changes Oct 14, 2022

View reviewed changes

stIncMale merged commit fbc5e4e into mongodb:master Oct 14, 2022

stIncMale deleted the JAVA-4701 branch October 14, 2022 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate the original error for write errors labeled `NoWritesPerformed` #1013

Propagate the original error for write errors labeled `NoWritesPerformed` #1013

stIncMale commented Oct 8, 2022

jyemin commented Oct 8, 2022

jyemin left a comment

stIncMale Oct 12, 2022

jyemin Oct 12, 2022

stIncMale commented Oct 12, 2022

jyemin Oct 12, 2022

jyemin commented Oct 12, 2022

jyemin left a comment

Propagate the original error for write errors labeled NoWritesPerformed #1013

Propagate the original error for write errors labeled NoWritesPerformed #1013

Conversation

stIncMale commented Oct 8, 2022

jyemin commented Oct 8, 2022

jyemin left a comment

Choose a reason for hiding this comment

stIncMale Oct 12, 2022

Choose a reason for hiding this comment

jyemin Oct 12, 2022

Choose a reason for hiding this comment

stIncMale commented Oct 12, 2022

jyemin Oct 12, 2022

Choose a reason for hiding this comment

jyemin commented Oct 12, 2022

jyemin left a comment

Choose a reason for hiding this comment

Propagate the original error for write errors labeled `NoWritesPerformed` #1013

Propagate the original error for write errors labeled `NoWritesPerformed` #1013