aminghadersohi commented on code in PR #44714:
URL: https://github.com/apache/superset/pull/44714#discussion_r4131050478
##########
superset/db_engine_specs/impala.py:
##########
@@ -128,6 +128,27 @@ def execute(
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex) from ex
+ @classmethod
+ def fetch_data(cls, cursor: Any, limit: int | None = None) ->
list[tuple[Any, ...]]:
+ """Wait for asynchronous operations using the public cursor API."""
+ if callable(getattr(cursor, "execute_async", None)):
+ from impala.error import Error
+
+ deadline = time.monotonic() + app.config["SQLLAB_TIMEOUT"]
+ try:
+ while cursor.is_executing():
+ if time.monotonic() >= deadline:
+ cursor.cancel_operation()
+ raise TimeoutError("Timed out waiting for the Impala
operation")
+ time.sleep(0.1)
Review Comment:
Reviewed at 792e3b626d654488afa52fb57b1707efcfa66a2c and compared both PR
diffs.
The loop at `superset/db_engine_specs/impala.py:137-143` **is introduced by
#44714**, but a synchronous wait before inspecting the result metadata is
intentional: `execute()` calls `execute_async()`, and returning an empty result
before completion would hide asynchronous DDL/DML failures. The wait sleeps
between polls and has a monotonic `SQLLAB_TIMEOUT` deadline; an adaptive
cadence would be a polling optimization, not a fix for an unbounded busy-wait.
The existing `handle_cursor()` does not cover every unfinished state (lines
157-160 omit `PENDING_STATE`). #44737 separately fixes pending-state polling
and live-cursor SQL Lab cancellation, including stopped/timed-out status and
cleanup after cancellation errors. I am keeping those changes in #44737 rather
than duplicating its cancel path here.
The proposed cancellation race does not match [impyla 0.24.0's
implementation](https://github.com/cloudera/impyla/blob/v0.24.0/impala/hiveserver2.py):
`is_executing()` (lines 514-518) reads status without clearing
`_last_operation_active`; server-side completion alone therefore does not
disable `cancel_operation()` (lines 322-327). A prior `_reset_state()` clears
`_last_operation` (lines 334-344), so the preceding `is_executing()` raises
rather than entering this timeout branch. If cancellation returns
normally/no-ops, line 142 still raises `TimeoutError`. If it raises driver
`Error`, lines 148-149 explicitly propagate the mapped database exception with
its cause; it is neither swallowed nor misreported as a timeout.
Resolving without a code change: the cadence suggestion is optional, the
alleged no-op/error-swallowing defect is not demonstrated, and the separate SQL
Lab pending-operation cancellation work remains in #44737. Validation:
inspected both PR diffs and the referenced driver implementation; no tests run
(no code changes).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
