-
Notifications
You must be signed in to change notification settings - Fork 71
feat: supported attach hint #833
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
Changes from all commits
81309ea
86936c8
44ed3e0
dae21de
4337854
db10c46
7c4f60e
17cf9da
71b827d
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 |
|---|---|---|
| @@ -0,0 +1,199 @@ | ||
| import asyncio | ||
| from unittest import mock | ||
|
|
||
| import pytest | ||
|
|
||
| from ydb._grpc.common.protos import ydb_query_pb2 | ||
| from ydb._grpc.grpcwrapper import common_utils | ||
| from ydb.aio.pool import ConnectionPool as AsyncConnectionPool | ||
| from ydb.pool import ConnectionPool, ConnectionsCache | ||
| from ydb.query.session import QuerySession | ||
|
|
||
|
|
||
| def _make_session(node_id=42): | ||
| driver = mock.Mock() | ||
| driver._pessimize_node = mock.Mock() | ||
| session = QuerySession(driver) | ||
| session._session_id = "test-session" | ||
| session._node_id = node_id | ||
| return session, driver | ||
|
|
||
|
|
||
| class TestQuerySessionAttachHints: | ||
| def test_node_shutdown_pessimizes_node_and_invalidates_session(self): | ||
| session, driver = _make_session(node_id=42) | ||
|
|
||
| session._handle_attach_session_state( | ||
| ydb_query_pb2.SessionState( | ||
| status=0, | ||
| node_shutdown=ydb_query_pb2.NodeShutdownHint(), | ||
| ) | ||
| ) | ||
|
|
||
| driver._pessimize_node.assert_called_once_with(42) | ||
| assert session._invalidated | ||
| assert session._closed | ||
|
|
||
| def test_session_shutdown_invalidates_without_pessimizing_node(self): | ||
| session, driver = _make_session(node_id=42) | ||
|
|
||
| session._handle_attach_session_state( | ||
| ydb_query_pb2.SessionState( | ||
| status=0, | ||
| session_shutdown=ydb_query_pb2.SessionShutdownHint(), | ||
| ) | ||
| ) | ||
|
|
||
| driver._pessimize_node.assert_not_called() | ||
| assert session._invalidated | ||
| assert session._closed | ||
|
|
||
| def test_node_shutdown_with_zero_node_id_delegates_to_driver(self): | ||
| session, driver = _make_session(node_id=0) | ||
|
|
||
| session._handle_attach_session_state( | ||
| ydb_query_pb2.SessionState( | ||
| status=0, | ||
| node_shutdown=ydb_query_pb2.NodeShutdownHint(), | ||
| ) | ||
| ) | ||
|
|
||
| driver._pessimize_node.assert_called_once_with(0) | ||
| assert session._invalidated | ||
|
|
||
| def test_node_shutdown_without_node_id_skips_pessimization(self): | ||
| session, driver = _make_session(node_id=None) | ||
|
|
||
| session._handle_attach_session_state( | ||
| ydb_query_pb2.SessionState( | ||
| status=0, | ||
| node_shutdown=ydb_query_pb2.NodeShutdownHint(), | ||
| ) | ||
| ) | ||
|
|
||
| driver._pessimize_node.assert_not_called() | ||
| assert session._invalidated | ||
| assert session._closed | ||
|
|
||
| def test_no_hint_does_not_invalidate(self): | ||
| session, driver = _make_session() | ||
|
|
||
| session._handle_attach_session_state( | ||
| ydb_query_pb2.SessionState(status=0), | ||
| ) | ||
|
|
||
| driver._pessimize_node.assert_not_called() | ||
| assert not session._invalidated | ||
| assert not session._closed | ||
|
|
||
| def test_none_response_does_not_invalidate(self): | ||
| session, driver = _make_session() | ||
|
|
||
| session._handle_attach_session_state(None) | ||
|
|
||
| driver._pessimize_node.assert_not_called() | ||
| assert not session._invalidated | ||
| assert not session._closed | ||
|
|
||
| def test_attach_stream_wrapper_handles_hint_and_returns_status(self): | ||
| session, driver = _make_session(node_id=42) | ||
| response = ydb_query_pb2.SessionState( | ||
| status=0, | ||
| session_shutdown=ydb_query_pb2.SessionShutdownHint(), | ||
| ) | ||
|
|
||
| status = session._attach_stream_wrapper(response) | ||
|
|
||
| driver._pessimize_node.assert_not_called() | ||
| assert session._invalidated | ||
| assert isinstance(status, common_utils.ServerStatus) | ||
|
|
||
| def test_attach_stream_wrapper_returns_status_without_hint(self): | ||
| session, driver = _make_session() | ||
| response = ydb_query_pb2.SessionState(status=0) | ||
|
|
||
| status = session._attach_stream_wrapper(response) | ||
|
|
||
| driver._pessimize_node.assert_not_called() | ||
| assert not session._invalidated | ||
| assert isinstance(status, common_utils.ServerStatus) | ||
|
|
||
|
|
||
| class TestConnectionPoolAttachHintPessimization: | ||
| def test_connections_cache_get_connection_by_node_id(self): | ||
| cache = ConnectionsCache() | ||
| connection = mock.Mock() | ||
| connection.endpoint = "grpc://localhost:2135" | ||
| connection.node_id = 42 | ||
| connection.add_cleanup_callback = mock.Mock() | ||
|
|
||
| cache.add(connection) | ||
|
|
||
| assert cache.get_connection_by_node_id(42) is connection | ||
| assert cache.get_connection_by_node_id(99) is None | ||
|
|
||
| def test_sync_pool_pessimizes_node_connection(self): | ||
| pool = ConnectionPool.__new__(ConnectionPool) | ||
| connection = mock.Mock() | ||
| pool._store = mock.Mock() | ||
| pool._store.get_connection_by_node_id.return_value = connection | ||
| pool._on_disconnected = mock.Mock() | ||
|
|
||
| pool._pessimize_node(42) | ||
|
|
||
| pool._store.get_connection_by_node_id.assert_called_once_with(42) | ||
| pool._on_disconnected.assert_called_once_with(connection) | ||
|
|
||
| def test_sync_pool_ignores_missing_node_connection(self): | ||
| pool = ConnectionPool.__new__(ConnectionPool) | ||
| pool._store = mock.Mock() | ||
| pool._store.get_connection_by_node_id.return_value = None | ||
| pool._on_disconnected = mock.Mock() | ||
|
|
||
| pool._pessimize_node(42) | ||
| pool._pessimize_node(0) | ||
| pool._pessimize_node(-1) | ||
|
|
||
| pool._store.get_connection_by_node_id.assert_called_once_with(42) | ||
| pool._on_disconnected.assert_not_called() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_async_pool_ignores_non_positive_node_id(self): | ||
| pool = AsyncConnectionPool.__new__(AsyncConnectionPool) | ||
| pool._store = mock.Mock() | ||
| pool._on_disconnected = mock.Mock() | ||
|
|
||
| pool._pessimize_node(0) | ||
| pool._pessimize_node(-1) | ||
|
|
||
| pool._store.get_connection_by_node_id.assert_not_called() | ||
| pool._on_disconnected.assert_not_called() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_async_pool_ignores_missing_node_connection(self): | ||
| pool = AsyncConnectionPool.__new__(AsyncConnectionPool) | ||
| pool._store = mock.Mock() | ||
| pool._store.get_connection_by_node_id.return_value = None | ||
| pool._on_disconnected = mock.Mock() | ||
|
|
||
| pool._pessimize_node(42) | ||
| await asyncio.sleep(0) | ||
|
|
||
| pool._store.get_connection_by_node_id.assert_called_once_with(42) | ||
| pool._on_disconnected.assert_not_called() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_async_pool_pessimizes_node_connection(self): | ||
| pool = AsyncConnectionPool.__new__(AsyncConnectionPool) | ||
| connection = mock.Mock() | ||
| disconnect = mock.AsyncMock() | ||
| pool._store = mock.Mock() | ||
| pool._store.get_connection_by_node_id.return_value = connection | ||
| pool._on_disconnected = mock.Mock(return_value=disconnect) | ||
|
|
||
| pool._pessimize_node(42) | ||
| await asyncio.sleep(0) | ||
|
|
||
| pool._store.get_connection_by_node_id.assert_called_once_with(42) | ||
| pool._on_disconnected.assert_called_once_with(connection) | ||
| disconnect.assert_awaited_once_with() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| import asyncio | ||
| import logging | ||
| import random | ||
| from typing import Any, Callable, Optional, Tuple, TYPE_CHECKING | ||
| from typing import Any, Callable, Optional, Tuple, TYPE_CHECKING, cast | ||
|
|
||
| from ydb import issues | ||
| from ydb.opentelemetry.tracing import SpanName, create_ydb_span | ||
|
|
@@ -311,6 +311,15 @@ async def __wrapper__() -> None: | |
|
|
||
| return __wrapper__ | ||
|
|
||
| def _pessimize_node(self, node_id: int) -> None: | ||
| """Deprioritize the connection attached to the given YDB node.""" | ||
| if node_id <= 0: | ||
| return | ||
|
|
||
| connection = cast(Optional[Connection], self._store.get_connection_by_node_id(node_id)) | ||
| if connection is not None: | ||
| asyncio.get_running_loop().create_task(self._on_disconnected(connection)()) | ||
|
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. Severity: Minor The task created by Additionally, if Suggested fix: store the task and add a done-callback to discard it, e.g.: task = asyncio.get_running_loop().create_task(self._on_disconnected(connection)())
task.add_done_callback(lambda t: t.exception() if not t.cancelled() else None)Or collect tasks in a set with |
||
|
|
||
| async def wait(self, timeout: Optional[float] = 7.0, fail_fast: bool = False) -> None: # type: ignore[override] # async override of sync method | ||
| with create_ydb_span(SpanName.DRIVER_INITIALIZE, self._driver_config, kind="internal").attach_context(): | ||
| await self._store.get(fast_fail=fail_fast, wait_timeout=timeout if timeout is not None else 7.0) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,6 +156,24 @@ def _check_session_ready_to_use(self) -> None: | |
| if self._closed: | ||
| raise RuntimeError(f"Session is not active, session_id: {self._session_id}, closed: {self._closed}") | ||
|
|
||
| def _attach_stream_wrapper(self, response_pb): | ||
| """Map attach-stream protobuf frames to ServerStatus and handle session hints.""" | ||
| self._handle_attach_session_state(response_pb) | ||
| return common_utils.ServerStatus.from_proto(response_pb) | ||
|
|
||
| def _handle_attach_session_state(self, response_pb) -> 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. Severity: Nit When a Suggested fix — add a debug-level log at the top of each branch: if hint == "node_shutdown":
logger.debug("NodeShutdown hint received, session_id: %s, node_id: %s", self._session_id, self._node_id)
...
elif hint == "session_shutdown":
logger.debug("SessionShutdown hint received, session_id: %s", self._session_id)
... |
||
| """Retire the session when the server sends a shutdown hint on the attach stream.""" | ||
| if response_pb is None: | ||
| return | ||
|
|
||
| hint = response_pb.WhichOneof("session_hint") | ||
| if hint == "node_shutdown": | ||
| if self._node_id is not None: | ||
| self._driver._pessimize_node(self._node_id) | ||
|
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. Severity: Minor If Suggested fix — wrap the pessimization so if hint == "node_shutdown":
if self._node_id is not None:
try:
self._driver._pessimize_node(self._node_id)
except Exception:
logger.debug("Failed to pessimize node %s", self._node_id, exc_info=True)
self._close_session(invalidate=True) |
||
| self._close_session(invalidate=True) | ||
| elif hint == "session_shutdown": | ||
| self._close_session(invalidate=True) | ||
|
|
||
| def _close_session(self, invalidate: bool = False) -> None: | ||
| if self._closed: | ||
| return | ||
|
|
@@ -362,7 +380,7 @@ def _attach(self, first_resp_timeout: int = DEFAULT_INITIAL_RESPONSE_TIMEOUT) -> | |
| self._stream = self._attach_call() | ||
| status_stream = _utilities.SyncResponseIterator( | ||
| self._stream, | ||
| lambda response: common_utils.ServerStatus.from_proto(response), | ||
| self._attach_stream_wrapper, | ||
| ) | ||
|
|
||
| try: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.