Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Dec 15, 2020
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ea37679
PYTHON-2452 Ensure RetryableWriteError error label return by MongoDB …
prashantmital Dec 7, 2020
e27166e
review suggestions
prashantmital Dec 8, 2020
1b17169
more review suggestions
prashantmital Dec 10, 2020
a03be35
more review
prashantmital Dec 10, 2020
69b3033
more more more review
prashantmital Dec 10, 2020
e7c9968
changes
prashantmital Dec 10, 2020
afbde7f
run only on RS
prashantmital Dec 10, 2020
94ca2f8
review changes
prashantmital Dec 10, 2020
1aaff7a
more review
prashantmital Dec 14, 2020
5a3bef9
add call to start retryable write
prashantmital Dec 14, 2020
8cb236f
fix test
prashantmital Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions pymongo/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,11 @@ def _check_command_response(response, max_wire_version,
max_wire_version)

if parse_write_concern_error and 'writeConcernError' in response:
_raise_write_concern_error(response['writeConcernError'])
_error = response["writeConcernError"]
_labels = response.get("errorLabels", [])
if _labels:
_error.update({'errorLabels': _labels})
_raise_write_concern_error(_error)

if response["ok"]:
return
Expand Down Expand Up @@ -221,8 +225,11 @@ def _check_write_command_response(result):
if write_errors:
_raise_last_write_error(write_errors)

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


Expand Down
31 changes: 30 additions & 1 deletion test/test_retryable_writes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@

from pymongo.errors import (ConnectionFailure,
OperationFailure,
ServerSelectionTimeoutError)
ServerSelectionTimeoutError,
WriteConcernError)
from pymongo.mongo_client import MongoClient
from pymongo.operations import (InsertOne,
DeleteMany,
Expand Down Expand Up @@ -453,6 +454,34 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs @require_failCommand_fail_point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'])

@client_context.require_failCommand_fail_point
def test_retryable_write_error_label_is_propagated(self):
listener = OvertCommandListener()
client = rs_or_single_client(
retryWrites=True, event_listeners=[listener])

fail_insert = {
'configureFailPoint': 'failCommand',
'mode': {'times': 2},
'data': {
'failCommands': ['insert'],
'closeConnection': False,
'writeConcernError': {
'code': 91,
'errmsg': 'Replication is being shut down'},
}}

with self.fail_point(fail_insert):
with self.assertRaises(WriteConcernError) as cm:
client.pymongo_test.testcoll.insert_one({})
self.assertIn('RetryableWriteError',
cm.exception._error_labels)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the has_error_label api.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


self.assertIn(
'RetryableWriteError',
listener.results['succeeded'][-1].reply['errorLabels'])


# TODO: Make this a real integration test where we stepdown the primary.
class TestRetryableWritesTxnNumber(IgnoreDeprecationsTest):
Expand Down