-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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-3587 Do not perform server selection to determine sessions support #1491
Changes from 14 commits
e68438f
c855a4d
b8240ea
f94ad1b
8edb2d3
589a2c8
0a8f605
6144aef
2cf1f66
66bcf90
2f9bb20
4d9b965
9b91b5c
0bbf30e
c2cee78
ff38be2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -752,6 +752,7 @@ def __init__( | |
self.active = False | ||
self.last_timeout = self.opts.socket_timeout | ||
self.connect_rtt = 0.0 | ||
self.logical_session_timeout_minutes: int = 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's default this to None or avoid confusing the 0 and None cases. (It's technically possible to set logical_session_timeout_minutes=0.) |
||
|
||
def set_conn_timeout(self, timeout: Optional[float]) -> None: | ||
"""Cache last timeout to avoid duplicate calls to conn.settimeout.""" | ||
|
@@ -870,6 +871,7 @@ def _hello( | |
self.max_message_size = hello.max_message_size | ||
self.max_write_batch_size = hello.max_write_batch_size | ||
self.supports_sessions = hello.logical_session_timeout_minutes is not None | ||
self.logical_session_timeout_minutes = hello.logical_session_timeout_minutes or 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove the |
||
self.hello_ok = hello.hello_ok | ||
self.is_repl = hello.server_type in ( | ||
SERVER_TYPE.RSPrimary, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -243,8 +243,7 @@ def is_server_type_known(self) -> bool: | |
def retryable_writes_supported(self) -> bool: | ||
"""Checks if this server supports retryable writes.""" | ||
return ( | ||
self._ls_timeout_minutes is not None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove _ls_timeout_minutes and all code for it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe so, there's no more use for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, according to the spec https://github.com/mongodb/specifications/blob/master/source/sessions/driver-sessions.rst, we still need to store the lowest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, so we still need to track it so we can use the lowest value for the server session cache expiration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bingo. |
||
and self._server_type in (SERVER_TYPE.Mongos, SERVER_TYPE.RSPrimary) | ||
self._server_type in (SERVER_TYPE.Mongos, SERVER_TYPE.RSPrimary) | ||
) or self._server_type == SERVER_TYPE.LoadBalancer | ||
|
||
@property | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,6 @@ | |
from pymongo import _csot, common, helpers, periodic_executor | ||
from pymongo.client_session import _ServerSession, _ServerSessionPool | ||
from pymongo.errors import ( | ||
ConfigurationError, | ||
ConnectionFailure, | ||
InvalidOperation, | ||
NetworkTimeout, | ||
|
@@ -47,7 +46,6 @@ | |
Selection, | ||
any_server_selector, | ||
arbiter_server_selector, | ||
readable_server_selector, | ||
secondary_server_selector, | ||
writable_server_selector, | ||
) | ||
|
@@ -579,38 +577,10 @@ def pop_all_sessions(self) -> list[_ServerSession]: | |
with self._lock: | ||
return self._session_pool.pop_all() | ||
|
||
def _check_implicit_session_support(self) -> None: | ||
with self._lock: | ||
self._check_session_support() | ||
|
||
def _check_session_support(self) -> float: | ||
"""Internal check for session support on clusters.""" | ||
if self._settings.load_balanced: | ||
# Sessions never time out in load balanced mode. | ||
return float("inf") | ||
session_timeout = self._description.logical_session_timeout_minutes | ||
if session_timeout is None: | ||
# Maybe we need an initial scan? Can raise ServerSelectionError. | ||
if self._description.topology_type == TOPOLOGY_TYPE.Single: | ||
if not self._description.has_known_servers: | ||
self._select_servers_loop( | ||
any_server_selector, self.get_server_selection_timeout(), None | ||
) | ||
elif not self._description.readable_servers: | ||
self._select_servers_loop( | ||
readable_server_selector, self.get_server_selection_timeout(), None | ||
) | ||
|
||
session_timeout = self._description.logical_session_timeout_minutes | ||
if session_timeout is None: | ||
raise ConfigurationError("Sessions are not supported by this MongoDB deployment") | ||
return session_timeout | ||
|
||
def get_server_session(self) -> _ServerSession: | ||
def get_server_session(self, session_timeout_minutes: Optional[int]) -> _ServerSession: | ||
"""Start or resume a server session, or raise ConfigurationError.""" | ||
with self._lock: | ||
session_timeout = self._check_session_support() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove _check_session_support? |
||
return self._session_pool.get_server_session(session_timeout) | ||
return self._session_pool.get_server_session(session_timeout_minutes) | ||
|
||
def return_server_session(self, server_session: _ServerSession, lock: bool) -> None: | ||
if lock: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,16 +116,16 @@ def _test_ops(self, client, *ops): | |
|
||
for f, args, kw in ops: | ||
with client.start_session() as s: | ||
listener.reset() | ||
s._materialize() | ||
last_use = s._server_session.last_use | ||
start = time.monotonic() | ||
self.assertLessEqual(last_use, start) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any other tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we can fix the issue by materializing the session before accessing last_use. |
||
listener.reset() | ||
# In case "f" modifies its inputs. | ||
args = copy.copy(args) | ||
kw = copy.copy(kw) | ||
kw["session"] = s | ||
f(*args, **kw) | ||
self.assertGreaterEqual(s._server_session.last_use, start) | ||
self.assertGreaterEqual(len(listener.started_events), 1) | ||
for event in listener.started_events: | ||
self.assertTrue( | ||
|
@@ -274,6 +274,8 @@ def test_end_sessions(self): | |
client = rs_or_single_client(event_listeners=[listener]) | ||
# Start many sessions. | ||
sessions = [client.start_session() for _ in range(_MAX_END_SESSIONS + 1)] | ||
for s in sessions: | ||
s._materialize() | ||
for s in sessions: | ||
s.end_session() | ||
|
||
|
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 remove this line?
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.
And just check
conn.supports_sessions
instead?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.
We can remove two of the variables in the check, the remaining two are required for tests to pass.