Skip to content

Commit c77f275

Browse files
Fix kernel async statement telemetry handle (#923)
* Fix kernel async statement telemetry handle Signed-off-by: Jay Xiao <jay.xiao@databricks.com> * Disable Python telemetry for kernel connections Signed-off-by: Jay Xiao <jay.xiao@databricks.com> * Use owning async handle for kernel query state Signed-off-by: Jay Xiao <jay.xiao@databricks.com> * Revert kernel telemetry helper guard Signed-off-by: Jay Xiao <jay.xiao@databricks.com> * Tighten kernel async handle comments Signed-off-by: Jay Xiao <jay.xiao@databricks.com> * ai: apply changes for #923 (2 review threads) Addresses: - #3832976351 at src/databricks/sql/backend/kernel/client.py:793 - #3836811357 at src/databricks/sql/backend/kernel/client.py:715 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for #923 (1 review thread) Addresses: - #3836884412 at src/databricks/sql/backend/kernel/client.py:816 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for #923 (1 review thread) Addresses: - #3836901963 at src/databricks/sql/backend/kernel/client.py:716 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for #923 (1 review thread) Addresses: - #3836923314 at src/databricks/sql/backend/kernel/client.py:729 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> * ai: apply changes for #923 (1 review thread) Addresses: - #3836943236 at src/databricks/sql/backend/kernel/client.py:823 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> --------- Signed-off-by: Jay Xiao <jay.xiao@databricks.com> Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> Co-authored-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent c95ca3f commit c77f275

4 files changed

Lines changed: 321 additions & 58 deletions

File tree

src/databricks/sql/backend/kernel/client.py

Lines changed: 124 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import logging
2626
import threading
2727
import uuid
28-
from typing import Any, Dict, List, Optional, TYPE_CHECKING, Union
28+
from typing import Any, Dict, List, Optional, Set, TYPE_CHECKING, Union
2929

3030
from databricks.sql.backend.databricks_client import DatabricksClient
3131
from databricks.sql.backend.kernel._errors import (
@@ -251,16 +251,27 @@ def __init__(
251251
# concurrent cursors on the same connection don't race on submit /
252252
# close / close-session.
253253
#
254-
# This is a KEEP-ALIVE registry, not a state/result lookup: the
254+
# This is primarily a KEEP-ALIVE registry: the
255255
# submitting ``ExecutedAsyncStatement``'s ``Drop`` fires a
256256
# fire-and-forget ``close_statement``, which would kill the
257257
# still-running async query the moment the handle is dropped. We
258258
# retain it (and its parent ``Statement``) here so the live query
259-
# survives until an explicit close. ``get_query_state`` /
260-
# ``get_execution_result`` do NOT consult this map — they
261-
# re-attach to the statement by id (the server is the source of
262-
# truth for async state), so they work even cross-process.
259+
# survives until an explicit close. ``get_query_state`` and
260+
# ``get_execution_result`` use this owning handle before result
261+
# streaming starts so kernel async statement telemetry is
262+
# finalized on the original ``ExecuteStatementAsync`` telemetry
263+
# object, then fall back to attach-by-id for re-fetch /
264+
# cross-process cases.
263265
self._async_handles: Dict[str, Any] = {}
266+
self._async_result_stream_started: Set[str] = set()
267+
# Async ids whose owning-handle ``status()`` poll is currently in
268+
# flight. A second concurrent poll of the same id (before result
269+
# streaming is claimed) is routed to the attach-by-id fallback so
270+
# it gets a fresh kernel handle instead of racing ``status()`` on
271+
# the shared owning handle. Guarded by ``_async_handles_lock``;
272+
# each entry is transient (added before the poll, discarded in a
273+
# ``finally``).
274+
self._async_status_in_flight: Set[str] = set()
264275
# Parent ``Statement`` objects kept alive alongside async handles.
265276
# On the kernel, ``Statement.close()`` flips the validity flag on
266277
# the produced executed handle (see kernel
@@ -406,6 +417,8 @@ def close_session(self, session_id: SessionId) -> None:
406417
tracked_stmts = list(self._async_statements.items())
407418
self._async_handles.clear()
408419
self._async_statements.clear()
420+
self._async_result_stream_started.clear()
421+
self._async_status_in_flight.clear()
409422
for _, handle in tracked:
410423
# Per-handle close errors are non-fatal — PEP 249
411424
# discourages raising from session close — so log and
@@ -657,6 +670,8 @@ def close_command(self, command_id: CommandId) -> None:
657670
with self._async_handles_lock:
658671
handle = self._async_handles.pop(command_id.guid, None)
659672
stmt = self._async_statements.pop(command_id.guid, None)
673+
self._async_result_stream_started.discard(command_id.guid)
674+
self._async_status_in_flight.discard(command_id.guid)
660675
# Closing the handle below fires the server-side CloseStatement.
661676
# A subsequent ``get_query_state`` re-attaches by id and reads
662677
# ``CLOSED`` straight from the server — no connector-side
@@ -686,18 +701,53 @@ def close_command(self, command_id: CommandId) -> None:
686701
pass
687702

688703
def get_query_state(self, command_id: CommandId) -> CommandState:
689-
# Server is the source of truth for async command state. Re-attach
690-
# to the statement by its id and read the state the server reports
691-
# — no connector-side state to drift. SEA keys GetStatementStatus
692-
# purely on the id, so a statement the connector no longer holds a
693-
# handle for (or never held — a different process) is still
694-
# queryable. CLOSED comes straight from the server: after a
704+
# Server is the source of truth for async command state. Use the
705+
# retained owning handle before result streaming starts so kernel
706+
# async statement telemetry is finalized on the original
707+
# ExecuteStatementAsync telemetry object. The owning-handle path
708+
# is per-connection, not per-cursor: any cursor on the submitting
709+
# connection (including a fresh cursor resuming the id) resolves
710+
# the same owning handle until result streaming is claimed — see
711+
# the concurrency note below for the limits that places on
712+
# concurrent polling. Once result streaming has been claimed, or
713+
# when this connector genuinely never held the handle (a
714+
# cross-process / restarted-process resume), re-attach to the
715+
# statement by id. SEA keys GetStatementStatus purely on the id,
716+
# so a statement the connector no longer holds a handle for is
717+
# still queryable. CLOSED comes straight from the server: after a
695718
# statement is closed (DELETE) the server still returns 200
696719
# state=CLOSED until the result TTL elapses.
697720
if self._kernel_session is None:
698721
raise InterfaceError("get_query_state requires an open session.")
722+
# Concurrency note: the lock guards the _async_handles /
723+
# _async_result_stream_started / _async_status_in_flight bookkeeping only.
724+
# The retained owning handle it returns is a shared object, and
725+
# handle.status() below runs OUTSIDE the lock, so it is not safe to invoke
726+
# status() on one owning handle from two threads at once. Rather than leave
727+
# concurrent in-process polling of a single async id "unsupported" and
728+
# undefined, we reserve the owning handle for the first poller via
729+
# _async_status_in_flight: a second concurrent poll of the same id (before
730+
# result streaming is claimed) sees the id already in flight and falls
731+
# through to the attach-by-id path, getting its own fresh kernel handle —
732+
# preserving the pre-change behaviour where every caller re-attached by id
733+
# and status() ran on distinct objects. The reservation is transient
734+
# (discarded in the finally below), so serial polls still take the
735+
# telemetry-preserving owning-handle path.
736+
with self._async_handles_lock:
737+
handle = (
738+
None
739+
if (
740+
command_id.guid in self._async_result_stream_started
741+
or command_id.guid in self._async_status_in_flight
742+
)
743+
else self._async_handles.get(command_id.guid)
744+
)
745+
reserved_owning_handle = handle is not None
746+
if reserved_owning_handle:
747+
self._async_status_in_flight.add(command_id.guid)
699748
try:
700-
handle = self._kernel_session.attach_async_statement(command_id.guid)
749+
if handle is None:
750+
handle = self._kernel_session.attach_async_statement(command_id.guid)
701751
state, failure = handle.status()
702752
except Exception as exc:
703753
if _is_not_found(exc):
@@ -721,6 +771,14 @@ def get_query_state(self, command_id: CommandId) -> CommandState:
721771
# sync-fall-through behaviour.
722772
return CommandState.SUCCEEDED
723773
raise _wrap_kernel_exception("get_query_state", exc) from exc
774+
finally:
775+
# Release the owning-handle reservation once this poll's
776+
# status() has completed (or raised). Only the reserver clears
777+
# it, so a concurrent poll that fell through to attach-by-id
778+
# never touches another poller's reservation.
779+
if reserved_owning_handle:
780+
with self._async_handles_lock:
781+
self._async_status_in_flight.discard(command_id.guid)
724782
if state == "Failed" and failure is not None:
725783
# Surface server-reported failure as a database error so
726784
# the cursor's polling loop terminates with the right
@@ -743,28 +801,68 @@ def get_execution_result(
743801
command_id: CommandId,
744802
cursor: "Cursor",
745803
) -> "ResultSet":
746-
# Re-attach to the statement by id and await its result. SEA keys
747-
# GetStatementResult on the id, so this works whether or not the
748-
# connector still holds the submitting handle — and it's
749-
# inherently re-callable (each call attaches a fresh handle and
750-
# re-materialises the result stream), matching the Thrift backend
751-
# where the operation handle stays re-fetchable until an explicit
752-
# close. No connector-side handle lookup, so no
753-
# ``unknown command_id`` failure on a second call.
804+
# Prefer the original owning async handle for the first
805+
# in-process result stream. The kernel attaches the real
806+
# ExecuteStatementAsync telemetry to that handle; attached
807+
# handles intentionally use no-op telemetry, so always
808+
# re-attaching loses the SEA async statement row when the result
809+
# is drained. After the owning result stream has been started,
810+
# attach by id for re-fetch. This preserves the Thrift-parity
811+
# behavior where results remain re-callable until explicit close.
812+
#
813+
# Concurrency: the owning handle is shared, and ``await_result()``
814+
# below runs OUTSIDE the lock, so it must not run on the same
815+
# handle a concurrent ``get_query_state`` poll is already using
816+
# for ``status()``. Mirror that method's guard here — if a status
817+
# poll has the owning handle reserved (guid in
818+
# ``_async_status_in_flight``), fall through to attach-by-id and
819+
# get a fresh kernel handle, exactly as an in-flight peer poll
820+
# does. In the normal serial flow (poll to terminal, then fetch)
821+
# the reservation is already discarded, so the fetch still takes
822+
# the telemetry-preserving owning-handle path.
754823
#
755-
# ``attach_async_statement`` issues a GetStatementStatus to seed
756-
# the handle; a 404 (unknown / aged-out id) surfaces as a
757-
# NotFound KernelError mapped to ``ProgrammingError`` below via
758-
# ``_wrap_kernel_exception``.
824+
# If this process does not hold the owning handle (fresh cursor,
825+
# restarted process, already re-fetched, or a concurrent poll
826+
# holds it), ``attach_async_statement`` issues a
827+
# GetStatementStatus to seed the handle; a 404 (unknown / aged-out
828+
# id) surfaces as a NotFound KernelError mapped to
829+
# ``ProgrammingError`` below via ``_wrap_kernel_exception``.
759830
if self._kernel_session is None:
760831
raise InterfaceError("get_execution_result requires an open session.")
832+
with self._async_handles_lock:
833+
handle = (
834+
None
835+
if (
836+
command_id.guid in self._async_result_stream_started
837+
or command_id.guid in self._async_status_in_flight
838+
)
839+
else self._async_handles.get(command_id.guid)
840+
)
841+
uses_owning_handle = handle is not None
842+
if uses_owning_handle:
843+
self._async_result_stream_started.add(command_id.guid)
761844
try:
762-
handle = self._kernel_session.attach_async_statement(command_id.guid)
845+
if handle is None:
846+
handle = self._kernel_session.attach_async_statement(command_id.guid)
763847
stream = handle.await_result()
764848
except Exception as exc:
849+
if uses_owning_handle:
850+
with self._async_handles_lock:
851+
self._async_result_stream_started.discard(command_id.guid)
765852
raise _wrap_kernel_exception("get_execution_result", exc) from exc
766853
# ``KernelResultSet.__init__`` calls ``arrow_schema()`` which
767854
# can raise — map that to PEP 249 too.
855+
#
856+
# Unlike the ``await_result()`` failure above, we deliberately do
857+
# NOT discard the ``_async_result_stream_started`` marker here.
858+
# By this point ``await_result()`` has already succeeded, so the
859+
# owning handle's result stream has been started (and may be
860+
# partially consumed); re-awaiting that same handle on a retry is
861+
# not safe. Leaving the marker set routes any retry through the
862+
# attach-by-id fallback, which re-materialises a fresh stream.
863+
# The trade-off is that such a retry loses the async-statement
864+
# telemetry — an accepted, narrow gap limited to the case where
865+
# result-set construction fails after a successful await.
768866
try:
769867
return self._make_result_set(stream, cursor, command_id)
770868
except Exception as exc:

src/databricks/sql/backend/kernel/result_set.py

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -252,13 +252,10 @@ def close(self) -> None:
252252
# connection close path stays clean.
253253
logger.warning("Error closing kernel handle: %s", exc)
254254
# Honor the base ``ResultSet`` contract: notify the backend.
255-
# ``backend.close_command`` also drops the ``_async_handles``
256-
# entry and records the guid in ``_closed_commands`` — no
257-
# separate pop needed here. Sync-execute and metadata paths
258-
# never registered in ``_async_handles`` to begin with, and
259-
# ``get_execution_result`` pops the async path before the
260-
# result set is even constructed (see the M1 fix), so this
261-
# call is the single bookkeeping seam.
255+
# For async results, ``backend.close_command`` drops the
256+
# retained owning handle and parent Statement. Sync-execute and
257+
# metadata paths never registered in ``_async_handles`` to begin
258+
# with, so this call is tolerant bookkeeping for them.
262259
backend = cast("KernelDatabricksClient", self.backend)
263260
try:
264261
backend.close_command(self.command_id)

tests/e2e/test_kernel_backend.py

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -462,14 +462,14 @@ def test_dml_rowcount_wiring_does_not_break_dml(conn):
462462
cur.execute(f"DROP TABLE IF EXISTS {tbl}")
463463

464464

465-
# ── Async execution: state + result come from the server (attach-by-id) ──
465+
# ── Async execution: owning handle first, attach-by-id for re-fetch/resume ──
466466

467467

468468
def test_async_execute_polls_and_fetches_result(conn):
469469
"""The full async CUJ: ``execute_async`` → poll
470-
``get_query_state`` → ``get_async_execution_result``. State and
471-
result are read from the server by re-attaching to the statement
472-
id (no connector-side state)."""
470+
``get_query_state`` → ``get_async_execution_result``. The first
471+
in-process flow uses the retained owning handle so kernel async
472+
telemetry is finalized."""
473473
with conn.cursor() as cur:
474474
cur.execute_async("SELECT 7 AS n")
475475
cur.get_async_execution_result() # polls to terminal, fetches
@@ -482,10 +482,9 @@ def test_async_execute_polls_and_fetches_result(conn):
482482

483483

484484
def test_async_get_execution_result_is_re_callable(conn):
485-
"""``get_async_execution_result`` re-attaches by id on each call,
486-
so fetching the same async command twice both succeed — the
487-
connector never relied on a one-shot retained handle (Thrift-parity
488-
re-fetch)."""
485+
"""Fetching the same async command twice succeeds: the first
486+
in-process result fetch can use the owning handle, and later
487+
re-fetches attach by id (Thrift-parity re-fetch)."""
489488
with conn.cursor() as cur:
490489
cur.execute_async("SELECT 11 AS n")
491490
cur.get_async_execution_result()

0 commit comments

Comments
 (0)