-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5169 - Deprecate Hedged Reads option #2213
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.
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
Conversation
pymongo/read_preferences.py
Outdated
@@ -103,6 +104,11 @@ def _validate_hedge(hedge: Optional[_Hedge]) -> Optional[_Hedge]: | |||
if not isinstance(hedge, dict): | |||
raise TypeError(f"hedge must be a dictionary, not {hedge!r}") | |||
|
|||
warnings.warn( | |||
"Hedged reads are deprecated starting in server version 8.0.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioning the server side deprecation is good but we should first mention the driver side deprecation. Something like:
the read preference "hedge" option is deprecated in PyMongo 4.12+ because hedged reads are deprecated in MongoDB version 8.0+. Support for "hedge" will be removed in PyMongo 5.0.
pyproject.toml
Outdated
@@ -119,6 +119,7 @@ filterwarnings = [ | |||
"module:please use dns.resolver.Resolver.resolve:DeprecationWarning", | |||
# https://github.com/dateutil/dateutil/issues/1314 | |||
"module:datetime.datetime.utc:DeprecationWarning", | |||
"module:Hedged reads are deprecated:DeprecationWarning", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using this approach we should manually catch/silence hedge errors only in tests that are intended to use it. We should also add a new test that we do raise the warning when hedge is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the changelog as well?
:class:`~pymongo.read_preferences.PrimaryPreferred`, | ||
:class:`~pymongo.read_preferences.Secondary`, | ||
:class:`~pymongo.read_preferences.SecondaryPreferred`, | ||
:class:`~pymongo.read_preferences.Nearest`. Support for ``hedge`` will be removed in PyMongo 5.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can hedge also appear in the URI/MongoClient kwarg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not listed in our docs page, so I don't believe so. You can create a read preference instance and pass that as a kwarg, but that will still pass through the read preference constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking
pymongo/read_preferences.py
Outdated
"The read preference 'hedge' option is deprecated in PyMongo 4.12+ because hedged reads are deprecated in MongoDB version 8.0+. Support for 'hedge' will be removed in PyMongo 5.0.", | ||
DeprecationWarning, | ||
stacklevel=2, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update all the hedge param docs to say:
:param hedge: **DEPRECATED** - The :attr:`~hedge` for this read preference.
@@ -203,6 +214,12 @@ def hedge(self) -> Optional[_Hedge]: | |||
|
|||
.. versionadded:: 3.11 | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update this docstring with **DEPRECATED**
and include the explanation as well.
pymongo/read_preferences.py
Outdated
@@ -143,6 +149,11 @@ def document(self) -> dict[str, Any]: | |||
if self.__max_staleness != -1: | |||
doc["maxStalenessSeconds"] = self.__max_staleness | |||
if self.__hedge not in (None, {}): | |||
warnings.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The document
helper should not raise a warning. We already raised a warning in the constructor. We do need to keep the warning in hedge()
since we will remove that api in 5.0.
pymongo/read_preferences.py
Outdated
warnings.warn( | ||
"The read preference 'hedge' option is deprecated in PyMongo 4.12+ because hedged reads are deprecated in MongoDB version 8.0+. Support for 'hedge' will be removed in PyMongo 5.0.", | ||
DeprecationWarning, | ||
stacklevel=2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last question. I'm wondering about the stacklevel here. Can you provide an example of the warning in the ReadPreference constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stacklevel=2
:
/Users/nstapp/Github/mongo-python-driver/pymongo/read_preferences.py:131: DeprecationWarning: The read preference 'hedge' option is deprecated in PyMongo 4.12+ because hedged reads are deprecated in MongoDB version 8.0+. Support for 'hedge' will be removed in PyMongo 5.0.
self.__hedge = _validate_hedge(hedge)
stacklevel=4
:
/Users/nstapp/Github/mongo-python-driver/test/asynchronous/test_read_preferences.py:583: DeprecationWarning: The read preference 'hedge' option is deprecated in PyMongo 4.12+ because hedged reads are deprecated in MongoDB version 8.0+. Support for 'hedge' will be removed in PyMongo 5.0.
cls(hedge={"enabled": True})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want stacklevel=4
to provide a useful traceback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it so.
No description provided.