PYTHON-2452 Ensure command-responses with RetryableWriteError label are retried on MongoDB 4.4+ #530

prashantmital · 2020-12-07T04:44:24Z

This change will be backported to 3.11 and 3.12 branches

…4.4+ is propagated to WriteConcernError exception

test/test_retryable_writes.py

ShaneHarvey · 2020-12-07T18:09:33Z

test/test_retryable_writes.py

@@ -453,6 +454,27 @@ def test_batch_splitting_retry_fails(self):
            self.assertEqual(final_txn, expected_txn)
        self.assertEqual(coll.find_one(projection={'_id': True}), {'_id': 1})

+    @client_context.require_version_min(4, 4)


This also needs @require_failCommand_fail_point

Why require 4.4? Shouldn't this test also work the same on 4.2?

This section https://github.com/mongodb/specifications/blob/master/source/retryable-writes/retryable-writes.rst#determining-retryable-errors seems to suggest that only MongoDB 4.4+ will add the RetryableWritesError to its command response. So the command assertion wouldn't work with MongoDB 4.2

That's correct but if I understand correctly, the spec says that on <4.4 the driver adds the RetryableWritesError label to retryable errors. So shouldn't the label be there either way?

Please update this to client_context.require_version_min(3, 6) to make sure we add the error label correctly on <4.4. We can then make the final assertion server version specific:

if version >= 4.4: self.assertIn( 'RetryableWriteError', listener.results['succeeded'][-1].reply['errorLabels'])

test/test_retryable_writes.py

ShaneHarvey · 2020-12-07T18:11:52Z

pymongo/helpers.py

-    error = result.get("writeConcernError")
+    error = result.get("writeConcernError", {})
+    error_labels = result.get("errorLabels", [])
+    error.update({'errorLabels': error_labels})


We should ensure that _check_command_response above has the same behavior. Also is it ever possible for error to be RawBSONDocument (where .update would fail)?

Added the behavior to _check_command_response. The question about RawBSONDocument needs further investigation.

This logic is flawed because it ends up always making error true. It should be:

error = result.get("writeConcernError", {}) if error: error_labels = result.get("errorLabels", []) if error_labels: error.update({'errorLabels': error_labels}) _raise_write_concern_error(error)

Good catch. Thanks.

This is not resolved because we still need to answer the RawBSONDocument question.

ShaneHarvey · 2020-12-10T00:38:07Z

pymongo/helpers.py

-        _raise_write_concern_error(response['writeConcernError'])
+        _error = response["writeConcernError"]
+        _labels = response.get("errorLabels", [])
+        _error.update({'errorLabels': _labels})


Should be:

if _labels: _error.update({'errorLabels': _labels})

So that we don't add an empty errorLabels to the error when the response doesn't have errorLabels.

ShaneHarvey · 2020-12-10T00:39:30Z

pymongo/helpers.py

-    error = result.get("writeConcernError")
+    error = result.get("writeConcernError", {})
+    error_labels = result.get("errorLabels", [])
+    error.update({'errorLabels': error_labels})


This logic is flawed because it ends up always making error true. It should be:

error = result.get("writeConcernError", {}) if error: error_labels = result.get("errorLabels", []) if error_labels: error.update({'errorLabels': error_labels}) _raise_write_concern_error(error)

ShaneHarvey · 2020-12-10T00:40:08Z

test/test_retryable_writes.py

@@ -453,6 +454,27 @@ def test_batch_splitting_retry_fails(self):
            self.assertEqual(final_txn, expected_txn)
        self.assertEqual(coll.find_one(projection={'_id': True}), {'_id': 1})

+    @client_context.require_version_min(4, 4)


Why require 4.4? Shouldn't this test also work the same on 4.2?

ShaneHarvey · 2020-12-10T00:41:46Z

test/test_retryable_writes.py

+
+        with self.fail_point(fail_insert):
+            with self.assertRaises(WriteConcernError) as cm:
+                client.testdb.testcoll.insert_one({})


I suggest using the standard pymongo_test database name here.

ShaneHarvey · 2020-12-10T19:16:51Z

test/test_retryable_writes.py

+            with self.assertRaises(WriteConcernError) as cm:
+                client.pymongo_test.testcoll.insert_one({})
+            self.assertIn('RetryableWriteError',
+                          cm.exception._error_labels)


This should use the has_error_label api.

ShaneHarvey · 2020-12-10T19:17:51Z

test/test_retryable_writes.py

@@ -453,6 +454,27 @@ def test_batch_splitting_retry_fails(self):
            self.assertEqual(final_txn, expected_txn)
        self.assertEqual(coll.find_one(projection={'_id': True}), {'_id': 1})

+    @client_context.require_version_min(4, 4)


Please update this to client_context.require_version_min(3, 6) to make sure we add the error label correctly on <4.4. We can then make the final assertion server version specific:

if version >= 4.4: self.assertIn( 'RetryableWriteError', listener.results['succeeded'][-1].reply['errorLabels'])

test/test_retryable_writes.py

prashantmital · 2020-12-10T22:41:07Z

Updated the test to account for https://jira.mongodb.org/browse/SERVER-39292 in MongoDB 4.0.x

ShaneHarvey

Did you get clarification from the server/retryable writes team on whether it's possible for errorLabels to appear in two places?

pymongo/helpers.py

test/test_retryable_writes.py

ShaneHarvey · 2020-12-11T00:14:37Z

Still waiting on the two questions on expected server behavior and RawBSONDocument.

prashantmital · 2020-12-14T19:51:03Z

RawBSONDocument will not break db.command because we set check_command_response to False
here which gets passed down here and ensures that we never check for the writeConcernError document within responses to db.command. I also quickly tested this by setting a failpoint that returned writeConcernError on insert and then triggering it via db.command.

Also, I confirmed that the server only returns errorLabels as a top-level field. See the HELP ticket linked to the JIRA ticket for details.

ShaneHarvey · 2020-12-14T19:55:22Z

Hmm, thanks for doing that investigation.

I also quickly tested this by setting a failpoint that returned writeConcernError on insert and then triggering it via db.command.

Can you add this as a test so that we don't regress on this behavior?

Edit: In particular, I've considered getting rid of the internal parse_write_concern_error option and instead always check for writeConcernError. If we did that I suspect this behavior might break and we wouldn't know without a test.

prashantmital · 2020-12-14T22:53:13Z

Added a test.

ShaneHarvey

LGTM

…re retried on MongoDB 4.4+ (#530) (cherry picked from commit f458473)

PYTHON-2452 Ensure RetryableWriteError error label return by MongoDB …

ea37679

…4.4+ is propagated to WriteConcernError exception

prashantmital requested a review from ShaneHarvey December 7, 2020 04:44

ShaneHarvey requested changes Dec 7, 2020

View reviewed changes

review suggestions

e27166e

ShaneHarvey reviewed Dec 10, 2020

View reviewed changes

more review suggestions

1b17169

ShaneHarvey requested changes Dec 10, 2020

View reviewed changes

prashantmital added 2 commits December 10, 2020 11:58

more review

a03be35

more more more review

69b3033

prashantmital requested a review from ShaneHarvey December 10, 2020 20:00

ShaneHarvey reviewed Dec 10, 2020

View reviewed changes

test/test_retryable_writes.py Outdated Show resolved Hide resolved

test/test_retryable_writes.py Outdated Show resolved Hide resolved

changes

e7c9968

prashantmital requested a review from ShaneHarvey December 10, 2020 22:41

run only on RS

afbde7f

ShaneHarvey reviewed Dec 10, 2020

View reviewed changes

pymongo/helpers.py Outdated Show resolved Hide resolved

ShaneHarvey reviewed Dec 10, 2020

View reviewed changes

test/test_retryable_writes.py Outdated Show resolved Hide resolved

review changes

94ca2f8

more review

1aaff7a

prashantmital added 2 commits December 14, 2020 15:13

add call to start retryable write

5a3bef9

fix test

8cb236f

ShaneHarvey approved these changes Dec 15, 2020

View reviewed changes

prashantmital changed the title ~~PYTHON-2452 Ensure RetryableWriteError error label return is propagated~~ PYTHON-2452 Ensure command-responses with RetryableWriteError label are retried on MongoDB 4.4+ Dec 15, 2020

prashantmital merged commit f458473 into mongodb:master Dec 15, 2020

prashantmital added a commit that referenced this pull request Dec 15, 2020

PYTHON-2452 Ensure command-responses with RetryableWriteError label a…

d808dae

…re retried on MongoDB 4.4+ (#530) (cherry picked from commit f458473)

prashantmital added a commit that referenced this pull request Dec 15, 2020

PYTHON-2452 Ensure command-responses with RetryableWriteError label a…

bc1d451

…re retried on MongoDB 4.4+ (#530) (cherry picked from commit f458473)

PYTHON-2452 Ensure command-responses with RetryableWriteError label are retried on MongoDB 4.4+ #530

PYTHON-2452 Ensure command-responses with RetryableWriteError label are retried on MongoDB 4.4+ #530

Uh oh!

Conversation

prashantmital commented Dec 7, 2020

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!

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!

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!

Uh oh!

Uh oh!

prashantmital commented Dec 10, 2020

Uh oh!

ShaneHarvey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ShaneHarvey commented Dec 11, 2020

Uh oh!

prashantmital commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShaneHarvey commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prashantmital commented Dec 14, 2020

Uh oh!

ShaneHarvey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prashantmital commented Dec 14, 2020 •

edited

Loading

ShaneHarvey commented Dec 14, 2020 •

edited

Loading