Skip to content

PYTHON-2143 Use an allow-list to determine resumable change stream errors #445

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

Jump to bottom
Merged
merged 8 commits into from
Jul 2, 2020
Merged

PYTHON-2143 Use an allow-list to determine resumable change stream errors #445

merged 8 commits into from
Jul 2, 2020

Conversation

prashantmital
Copy link
Contributor

No description provided.

behackett
behackett requested changes Jun 25, 2020
View reviewed changes
Copy link
Member

@behackett behackett left a comment

Choose a reason for hiding this comment

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

I'm not intending to review the code changes here, but please don't use the word whitelist (or blacklist) in the code or commit message.

@prashantmital prashantmital force-pushed the PYTHON-2143/change-streams-resume branch from c59c305 to b28dbc2 Compare June 26, 2020 01:22
prashantmital
prashantmital commented Jun 26, 2020
View reviewed changes
Copy link
Contributor Author

@prashantmital prashantmital left a comment

Choose a reason for hiding this comment

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

Still waiting for the outcome of the CursorNotFound discussion, but the main change (adding wire version information to OperationFailure instances) can now be reviewed.

@prashantmital prashantmital changed the title PYTHON-2143 Use a whitelist to determine resumable change stream errors PYTHON-2143 Use an allow-list to determine resumable change stream errors Jun 26, 2020
@prashantmital prashantmital marked this pull request as ready for review June 26, 2020 03:35
ShaneHarvey
ShaneHarvey reviewed Jun 26, 2020
View reviewed changes
pymongo/errors.py Outdated
@@ -167,6 +170,15 @@ def details(self):
"""
return self.__details

@property
def max_wire_version(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this private+undocumented (_ max_wire_version)? Do you see any reason to make it public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, no reason. I'll make it private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also removed the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

Can you make this private (again)? Somehow it got re-added.

pymongo/helpers.py Outdated
@@ -103,14 +103,16 @@ def _index_document(index_list):


def _check_command_response(response, msg=None, allowable_errors=None,
parse_write_concern_error=False):
parse_write_concern_error=False,
max_wire_version=None):
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we make max_wire_version a required argument to ensure we don't forget to pass it.

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. Note that I used None as the max_wire_version value in several tests that directly use this method.

prashantmital
prashantmital commented Jun 30, 2020
View reviewed changes
Copy link
Contributor Author

@prashantmital prashantmital left a comment

Choose a reason for hiding this comment

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

Addressed your suggestions.

pymongo/errors.py Outdated
@@ -167,6 +170,15 @@ def details(self):
"""
return self.__details

@property
def max_wire_version(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, no reason. I'll make it private.

pymongo/errors.py Outdated
@@ -167,6 +170,15 @@ def details(self):
"""
return self.__details

@property
def max_wire_version(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also removed the docstring.

pymongo/helpers.py Outdated
@@ -103,14 +103,16 @@ def _index_document(index_list):


def _check_command_response(response, msg=None, allowable_errors=None,
parse_write_concern_error=False):
parse_write_concern_error=False,
max_wire_version=None):
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. Note that I used None as the max_wire_version value in several tests that directly use this method.

ShaneHarvey
ShaneHarvey reviewed Jun 30, 2020
View reviewed changes
pymongo/errors.py Outdated
@@ -136,18 +136,25 @@ class ConfigurationError(PyMongoError):
class OperationFailure(PyMongoError):
"""Raised when a database operation fails.

.. versionadded:: 3.11
The :attr:`max_wire_version` attribute.
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this now.

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.

@prashantmital prashantmital requested a review from ShaneHarvey July 1, 2020 23:25
@prashantmital prashantmital force-pushed the PYTHON-2143/change-streams-resume branch from 4278190 to dd23624 Compare July 1, 2020 23:47
ShaneHarvey
ShaneHarvey reviewed Jul 1, 2020
View reviewed changes
pymongo/network.py Outdated
@@ -40,7 +40,7 @@
_UNPACK_HEADER = struct.Struct("<iiii").unpack


def command(sock_info, dbname, spec, slave_ok, is_mongos,
def command(sock_info, max_wire_version, dbname, spec, slave_ok, is_mongos,
Copy link
Member

Choose a reason for hiding this comment

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

Please remove max_wire_version and use sock_info.max_wire_version instead.

ShaneHarvey
ShaneHarvey requested changes Jul 2, 2020
View reviewed changes
pymongo/pool.py Outdated
self.is_mongos, read_preference, codec_options,
session, client, check, allowable_errors,
self.address, check_keys, listeners,
self.max_bson_size, read_concern,
Copy link
Member

Choose a reason for hiding this comment

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

Can you undo these changes?

ShaneHarvey
ShaneHarvey approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM

@prashantmital prashantmital merged commit 26913ea into mongodb:master Jul 2, 2020
@prashantmital prashantmital deleted the PYTHON-2143/change-streams-resume branch July 2, 2020 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behackett behackett behackett requested changes

@ShaneHarvey ShaneHarvey ShaneHarvey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@prashantmital @behackett @ShaneHarvey