feat: supported attach hint#833
Conversation
397fc60 to
a94ba81
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #833 +/- ##
==========================================
+ Coverage 81.30% 81.32% +0.02%
==========================================
Files 94 94
Lines 12162 12189 +27
Branches 1194 1202 +8
==========================================
+ Hits 9888 9913 +25
- Misses 1809 1810 +1
- Partials 465 466 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 87 changed files in this pull request and generated 2 comments.
Files not reviewed (13)
- ydb/_grpc/v3/draft/protos/ydb_maintenance_pb2.py: Language not supported
- ydb/_grpc/v3/draft/protos/ydb_replication_pb2.py: Language not supported
- ydb/_grpc/v3/draft/protos/ydb_view_pb2.py: Language not supported
- ydb/_grpc/v3/draft/ydb_replication_v1_pb2.py: Language not supported
- ydb/_grpc/v3/draft/ydb_view_v1_pb2.py: Language not supported
- ydb/_grpc/v3/protos/ydb_query_pb2.py: Language not supported
- ydb/_grpc/v3/protos/ydb_topic_pb2.py: Language not supported
- ydb/_grpc/v3/ydb_table_v1_pb2.py: Language not supported
- ydb/_grpc/v4/draft/protos/ydb_maintenance_pb2.py: Language not supported
- ydb/_grpc/v4/draft/protos/ydb_replication_pb2.py: Language not supported
- ydb/_grpc/v4/draft/protos/ydb_view_pb2.py: Language not supported
- ydb/_grpc/v4/draft/ydb_replication_v1_pb2.py: Language not supported
- ydb/_grpc/v4/draft/ydb_view_v1_pb2.py: Language not supported
🌋 SLO Test Results🟢 2 workload(s) tested — All thresholds passed
Generated by ydb-slo-action |
a0a70db to
3a583b1
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Move the missing-node-id check to the session and tighten the connection pool contract to accept a concrete int node id. Co-authored-by: Cursor <cursoragent@cursor.com>
Look up node connections under ConnectionsCache lock when pessimizing after NodeShutdown hints, and strip grpcio-tools version gates from v6 gRPC stubs so imports work with grpcio>=1.42.0. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore stubs from main and strip grpcio version gates via generate_protoc, instead of leaving broken conflict markers or truncated files. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore auto-generated stubs and generate_protoc.py to match main. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
80fb11a to
17cf9da
Compare
Cover attach stream wrapper, ConnectionsCache lookup, and async pool pessimize edge cases to satisfy codecov/patch threshold. Co-authored-by: Cursor <cursoragent@cursor.com>
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Minor | Medium: Fire-and-forget asyncio task reference not saved —
ydb/aio/pool.py:321 - Minor | Low: If
_pessimize_noderaises,_close_sessionis skipped on that code path —ydb/query/session.py:172 - Nit | High: No logging when session shutdown hints are received —
ydb/query/session.py:164
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
|
|
||
| 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.
Severity: Minor
Confidence: Medium
The task created by create_task() is not saved to a variable. Per the Python docs: "Save a reference to the result of this function, to avoid a task disappearing mid-execution." Starting from Python 3.12, the event loop only keeps weak references to tasks, so an unsaved task may be garbage-collected before completing.
Additionally, if connection.close() inside _on_disconnected raises, the exception will go unhandled (only producing a Task exception was never retrieved warning).
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 task.add_done_callback(self._background_tasks.discard).
| 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.
Severity: Minor
Confidence: Low
If _pessimize_node raises (e.g. connection.close() fails inside the sync pool's _on_disconnected), the _close_session(invalidate=True) call on line 173 is skipped. The session is eventually invalidated through the exception propagating up to _check_session_status_loop's except handler, but the recovery path is indirect.
Suggested fix — wrap the pessimization so _close_session is always reached:
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._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.
Severity: Nit
Confidence: High
When a NodeShutdown or SessionShutdown hint arrives, the session is closed without any log entry indicating why. The eventual stream-error log from _check_session_status_loop ("Attach stream error: …") doesn't mention the hint was the root cause, making it harder to diagnose graceful-shutdown scenarios in production.
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)
...
|
Analysis performed by claude, claude-opus-4-6. |
No description provided.