Skip to content
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

PYTHON-3879 Rename SocketInfo to Connection #1329

Merged
merged 7 commits into from
Jul 28, 2023

Conversation

NoahStapp
Copy link
Contributor

No description provided.

@NoahStapp NoahStapp requested a review from ShaneHarvey July 26, 2023 22:39
@@ -347,7 +347,7 @@ def wrap_socket(
server_hostname=None,
session=None,
):
"""Wrap an existing Python socket sock and return a TLS socket
"""Wrap an existing Python socket connector and return a TLS socket
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the renaming went a little too far.

pymongo/pool.py Outdated

@contextlib.contextmanager
def get_socket(self, handler=None):
"""Get a socket from the pool. Use with a "with" statement.
def get_conn(self, handler=None):
Copy link
Member

Choose a reason for hiding this comment

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

Since where changing this anyway, how do you feel about calling this "pool.checkout()" to closer to the CMAP spec?

pymongo/pool.py Outdated

def return_socket(self, sock_info):
"""Return the socket to the pool, or if it's closed discard it.
def return_conn(self, conn):
Copy link
Member

Choose a reason for hiding this comment

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

Same here but called "checkin"

@@ -134,7 +134,7 @@ def get_cursor(
self,
session: ClientSession,
server: Server,
sock_info: SocketInfo,
connection: Connection,
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about naming this "conn: Connection" to be more succinct everywhere?

@NoahStapp NoahStapp requested a review from ShaneHarvey July 27, 2023 18:20
@blink1073
Copy link
Member

There's still a lot of docstrings that have unnecessary changes.

@@ -1644,11 +1644,11 @@ def find(self, *args: Any, **kwargs: Any) -> Cursor[_DocumentType]:

- A :class:`~pymongo.cursor.Cursor` instance created with the
:attr:`~pymongo.cursor.CursorType.EXHAUST` cursor_type requires an
exclusive :class:`~socket.socket` connection to MongoDB. If the
exclusive :class:`~socket.socket` conn to MongoDB. If the
Copy link
Member

Choose a reason for hiding this comment

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

These three docstring changes should be reverted

@@ -175,7 +175,7 @@ def __init__(
quote_plus(user), quote_plus(password), host)
client = MongoClient(uri)

Unix domain sockets are also supported. The socket path must be percent
Unix domain conns are also supported. The socket path must be percent
Copy link
Member

Choose a reason for hiding this comment

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

Revert

@@ -184,7 +184,7 @@ def __init__(

But not when passed as a simple hostname::

client = MongoClient('/tmp/mongodb-27017.sock')
client = MongoClient('/tmp/mongodb-27017.conn')
Copy link
Member

Choose a reason for hiding this comment

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

Revert

@@ -308,18 +308,18 @@ def __init__(
seconds).
- `waitQueueTimeoutMS`: (integer or None) How long (in milliseconds)
a thread will wait for a socket from the pool if the pool has no
free sockets. Defaults to ``None`` (no timeout).
free conns. Defaults to ``None`` (no timeout).
Copy link
Member

Choose a reason for hiding this comment

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

Revert

query log and profile collections.
- `driver`: (pair or None) A driver implemented on top of PyMongo can
pass a :class:`~pymongo.driver_info.DriverInfo` to add its name,
version, and platform to the message printed in the server log when
establishing a connection.
establishing a conn.
Copy link
Member

Choose a reason for hiding this comment

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

Revert

@@ -914,7 +914,7 @@ class ConnectionCheckOutFailedReason:

CONN_ERROR = "connectionError"
"""The connection check out attempt experienced an error while setting up
a new connection.
a new conn.
Copy link
Member

Choose a reason for hiding this comment

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

Revert

@@ -1023,7 +1023,7 @@ def __repr__(self):


class ConnectionCheckOutStartedEvent(_ConnectionEvent):
"""Published when the driver starts attempting to check out a connection.
"""Published when the driver starts attempting to check out a conn.
Copy link
Member

Choose a reason for hiding this comment

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

revert (all the docstring changes in this file).

context = sock_info.cancel_context
# Only Monitor connections can be cancelled.
context = conn.cancel_context
# Only Monitor conns can be cancelled.
Copy link
Member

Choose a reason for hiding this comment

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

revert

@@ -309,33 +307,31 @@ def wait_for_read(sock_info: SocketInfo, deadline: Optional[float]) -> None:
raise socket.timeout("timed out")


# Errors raised by sockets (and TLS sockets) when in non-blocking mode.
# Errors raised by conns (and TLS conns) when in non-blocking mode.
Copy link
Member

Choose a reason for hiding this comment

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

revert

except BLOCKING_IO_ERRORS:
raise socket.timeout("timed out")
except OSError as exc: # noqa: B014
if _errno_from_exception(exc) == errno.EINTR:
continue
raise
if chunk_length == 0:
raise OSError("connection closed")
raise OSError("conn closed")
Copy link
Member

Choose a reason for hiding this comment

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

revert

pymongo/command_cursor.py Outdated Show resolved Hide resolved
@NoahStapp
Copy link
Contributor Author

Sorry for all the docstring issues, I figured out how to have PyCharm filter for that correctly. They should all be resolved now.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@NoahStapp NoahStapp merged commit c88ae79 into mongodb:master Jul 28, 2023
@NoahStapp NoahStapp deleted the PYTHON-3879 branch July 28, 2023 17:04
tabgok added a commit to DataDog/dd-trace-py that referenced this pull request Aug 28, 2023
….x (#6736)

Pymongo 4.5.0 [changed the
name](mongodb/mongo-python-driver#1329) of the
`get_socket` function on which our integration is based. This pull
request adapts the integration to work with version 4.5.0 by changing
the expected function name based on the version.

Fixes #6723

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Teague Bick <[email protected]>
github-actions bot pushed a commit to DataDog/dd-trace-py that referenced this pull request Aug 28, 2023
….x (#6736)

Pymongo 4.5.0 [changed the
name](mongodb/mongo-python-driver#1329) of the
`get_socket` function on which our integration is based. This pull
request adapts the integration to work with version 4.5.0 by changing
the expected function name based on the version.

Fixes #6723

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Teague Bick <[email protected]>
(cherry picked from commit 1a3c424)
mabdinur added a commit to DataDog/dd-trace-py that referenced this pull request Aug 28, 2023
… [backport 1.18] (#6775)

Backport 1a3c424 from #6736 to 1.18.

Pymongo 4.5.0 [changed the
name](mongodb/mongo-python-driver#1329) of the
`get_socket` function on which our integration is based. This pull
request adapts the integration to work with version 4.5.0 by changing
the expected function name based on the version.

Fixes #6723

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Emmett Butler <[email protected]>
Co-authored-by: Munir Abdinur <[email protected]>
juanjux pushed a commit to DataDog/dd-trace-py that referenced this pull request Aug 29, 2023
….x (#6736)

Pymongo 4.5.0 [changed the
name](mongodb/mongo-python-driver#1329) of the
`get_socket` function on which our integration is based. This pull
request adapts the integration to work with version 4.5.0 by changing
the expected function name based on the version.

Fixes #6723

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Teague Bick <[email protected]>
@msm416
Copy link

msm416 commented Aug 1, 2024

Hi, shouldn't this change be mentioned in the Changelog? Thanks!

@ShaneHarvey
Copy link
Member

Hi, shouldn't this change be mentioned in the Changelog? Thanks!

We did not mention it because SocketInfo/Connection are private internal apis. Are you depending it for something?

@msm416
Copy link

msm416 commented Aug 2, 2024

@ShaneHarvey - I see, it's alright since I've changed the import to workaround for now but will probably re-write my logic to not make use of those classes.

SocketInfo was documented in pymongo v3: https://pymongo.readthedocs.io/en/3.12.3/api/pymongo/pool.html

To me, private api == what's not documented, but I expected that if something is once documented but then removed from docs, that should appear in some changelogs for visibility.

@ShaneHarvey
Copy link
Member

ShaneHarvey commented Aug 2, 2024

Yes we removed that documentation in 4.0 (part of PYTHON-2164) but even in v3, those classes were still intended to be private. An app couldn't actually access a MongoClient's Pool or Connection classes so there was no reason to document those apis. I'd still be curious to hear what you're using those classes for.

@msm416
Copy link

msm416 commented Aug 3, 2024

[..] those classes were still intended to be private

I see.

I'd still be curious to hear what you're using those classes for.

I'm using the class in a test: to patch Connection.send_message with a custom method wrapper that counts the calls for this method, to give a just a brief description.

@ShaneHarvey
Copy link
Member

Interesting, could your test use a CommandListener instead https://pymongo.readthedocs.io/en/stable/api/pymongo/monitoring.html?

@msm416
Copy link

msm416 commented Aug 7, 2024

Thank you @ShaneHarvey ! Yes, I made it work with a Listener in the end, after following your docs (CommandLogger) 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants