-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5344 and PYTHON-5403 Allow Instantiated MongoClients to Send Client Metadata On-Demand #2358
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
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.
LGTM!
pymongo/asynchronous/mongo_client.py
Outdated
@@ -1040,6 +1041,23 @@ async def target() -> bool: | |||
self._kill_cursors_executor = executor | |||
self._opened = False | |||
|
|||
def append_metadata(self, driver_info: DriverInfo) -> None: | |||
""" | |||
Appends the given metadata to existing driver metadata. |
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 this docstring to follow our standard format (params, versionadded, etc...)?
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 made my best attempt at it, but lmk if I was off / am still missing something / have it incorrectly formatted >.<
|
||
import pytest | ||
|
||
from pymongo import AsyncMongoClient, MongoClient |
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.
from pymongo import AsyncMongoClient, MongoClient | |
from pymongo import AsyncMongoClient |
if "ismaster" in r: | ||
# then this is a handshake request | ||
self.handshake_req = r | ||
return r.reply(OpMsgReply(minWireVersion=0, maxWireVersion=13)) |
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.
Is maxWireVersion=13
specified by the spec or by MockupDB?
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 its needed so that MockupDB would work(?) without specification, the maxWireVersion
is 6, and PyMongo raises a configuration error: pymongo.errors.ConfigurationError: Server at localhost:56617 reports wire version 0, but this version of PyMongo requires at least 7 (MongoDB 4.0).
# send initial metadata | ||
name, version, platform, metadata = await self.send_ping_and_get_metadata(client, True) | ||
# wait for connection to become idle | ||
await asyncio.sleep(0.005) |
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 we refactor this into an async_wait_until
with some predicate?
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 spec specifically stated to wait 5ms
self.assertEqual(metadata, new_metadata) | ||
|
||
async def test_append_metadata(self): | ||
client = AsyncMongoClient( |
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.
All of these clients should be created with one of our PyMongoTestCase
helpers, not raw AsyncMongoClient()
.
test/asynchronous/unified_format.py
Outdated
@@ -925,7 +931,6 @@ async def run_entity_operation(self, spec): | |||
) | |||
else: | |||
arguments = {} | |||
|
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.
Whitespace removed?
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.
oops accident!
pymongo/pool_options.py
Outdated
def _update_metadata(self, driver: DriverInfo): | ||
"""Updates the client's metadata""" | ||
if driver.name: | ||
self.__metadata["driver"]["name"] = "{}|{}".format( |
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 we make this method atomic? As in, first create a copy of __metadata
, then mutate, then update? That way the driver handshake will never serialize half of the metadata.
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.
API changes LGTM. Could you also add the changelog entry?
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.
LGTM! Great work!
https://jira.mongodb.org/browse/PYTHON-5344
https://jira.mongodb.org/browse/PYTHON-5403 (ticket for async tests)
Notes:
test/mockupdb
) to utilize synchro for the async -> sync tests.