Fix kernel async statement telemetry handle - #923
Conversation
410999a to
7deecf2
Compare
7deecf2 to
6629d87
Compare
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the owning-handle-first / attach-by-id-fallback change is correct: the claim is check-and-set under _async_handles_lock, cleanup is consistent across close_command/close_session, and the new unit + e2e tests cover the main paths. One low-severity note on an unclear telemetry-finalization edge when result-set construction fails after a successful await_result().
| stream = handle.await_result() | ||
| except Exception as exc: | ||
| if uses_owning_handle: | ||
| with self._async_handles_lock: |
There was a problem hiding this comment.
🔵 Low — The owning-handle failure path only discards _async_result_stream_started when await_result() raises. If await_result() succeeds (marker stays set) but the subsequent KernelResultSet.__init__ → arrow_schema() raises and is re-wrapped, the guid remains marked as started. A later retry then takes the attach-by-id (no-op telemetry) branch.
Whether this loses the ExecuteStatementAsync telemetry row depends on when the kernel finalizes it: if finalization happens when await_result() returns, this is harmless (telemetry already committed). If finalization only completes once the result stream is drained, the telemetry is lost on this retry because the owning handle is never reused. The PR's own comments ("first in-process result stream", "clear the claimed marker so a retry can still use the telemetry-bearing owning handle") are ambiguous on this point, and the added test_get_execution_result_owning_handle_failure_can_retry_owning_handle only exercises the await_result()-raises case, not the construct-failure-after-await case. Worth confirming the finalization semantics and, if drain-based, discarding the marker on the construction-failure path too.
Summary
Testing