aminghadersohi commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4429787145
##########
superset/db_engine_specs/impala.py:
##########
@@ -136,30 +139,43 @@ def execute(
@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:
+ 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:
+ cls._cancel_operation(cursor)
+ raise TimeoutError(
+ "Timed out waiting for the Impala operation"
+ )
+ time.sleep(0.1)
+ if cursor.execution_failed():
+ # Fetching surfaces the driver's detailed asynchronous
error,
+ # including errors on statements that produce no
result set.
+ cursor.fetchall()
+ except Error as ex:
+ raise cls.get_dbapi_mapped_exception(ex) from ex
+ return super().fetch_data(cursor, limit)
+ except TIME_LIMIT_EXCEPTIONS:
+ # The operation is released before the time limit propagates so
+ # it does not keep running on the coordinator.
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)
- if cursor.execution_failed():
- # Fetching surfaces the driver's detailed asynchronous
error,
- # including errors on statements that produce no result
set.
- cursor.fetchall()
- except Error as ex:
- raise cls.get_dbapi_mapped_exception(ex) from ex
- return super().fetch_data(cursor, limit)
+ cursor.cancel_operation()
+ except TIME_LIMIT_EXCEPTIONS:
+ raise
+ except Exception: # pylint: disable=broad-except
+ logger.warning("cancel_operation() failed after a time limit")
+ raise
Review Comment:
Fixed the remaining polling path in
24860720185c06394e8eb4ccf5c0f4a30cea21b2. My earlier reply wrongly marked
handle_cursor (then impala.py:203) unaffected. A signal can interrupt any RPC
reply, including GetOperationStatus and GetLog, not just FetchResults. Both
time-limit handlers use one helper to reconnect, cancel, and release
operation/cursor handles before re-raising; non-time-limit paths are unchanged.
Regression test:
`test_handle_cursor_reconnects_before_cancelling_an_interrupted_poll` exercises
status/get_log, SoftTimeLimitExceeded/SupersetTimeoutException, and
successful/failed cancellation. Before the code change,
`PYTHONPATH=.:superset-core/src pytest -q
tests/unit_tests/db_engine_specs/test_impala.py -k interrupted_poll` returned
`8 failed, 29 deselected`; the assertion was `At index 0 diff:
call.cancel_operation() != call.reconnect()`. After the change, the Impala/Hive
suite returned `101 passed`, including that test and
`test_fetch_data_reconnects_before_cancelling_an_interrupted_fetch`.
Base/Presto neighbors returned `282 passed`. Staged-file and
branch-changed-file pre-commit checks passed, including mypy. Real Impala not
run; no live transport or server-side slot-release claim.
Search: `rg -n
'cancel_operation|_cancel_operation|SoftTimeLimitExceeded|TIME_LIMIT_EXCEPTIONS|def
handle_cursor|def fetch_data' superset/db_engine_specs superset/sql_lab.py`,
plus Spark inheritance.
Siblings checked:
- superset/db_engine_specs/impala.py:140 — fixed in
24860720185c06394e8eb4ccf5c0f4a30cea21b2; fetch_data uses the shared
reconnect-before-cancel helper for time limits during polling or row fetching.
- superset/db_engine_specs/impala.py:208 — fixed in
24860720185c06394e8eb4ccf5c0f4a30cea21b2; handle_cursor routes interrupted
status and log RPCs through the same helper.
- superset/db_engine_specs/impala.py:167 — fixed in
24860720185c06394e8eb4ccf5c0f4a30cea21b2; the shared helper reconnects before
_cancel_operation, preserving reconnect-failure logging and propagation of
another time limit.
- superset/db_engine_specs/impala.py:183 — not affected: the low-level
cancel/release helper is reached through reconnect for signal-driven
cancellation; ordinary RPC failures are logged, and another time limit
propagates rather than starting another RPC.
- superset/db_engine_specs/impala.py:150 — not affected: the monotonic
deadline is checked after is_executing returns, with no interrupted reply;
signal exceptions instead enter the reconnect handler.
- superset/db_engine_specs/impala.py:237 — not affected: explicit Stop is
checked after status returns, not from a signal interrupt; a time limit during
cancellation enters the outer reconnect handler.
- superset/db_engine_specs/impala.py:323 — not affected: cancel_query uses a
separate HTTP requester, not the interrupted impyla transport.
- superset/db_engine_specs/base.py:144 — not affected: TIME_LIMIT_EXCEPTIONS
only defines the exception tuple; it sends no RPC.
- superset/db_engine_specs/base.py:1624 — not affected: generic fetching
maps exceptions but issues no cancellation RPC after interruption; Impala wraps
it with the shared handler.
- superset/db_engine_specs/base.py:1670 — not affected: pagination entry
point raises NotImplementedError and sends no RPC.
- superset/db_engine_specs/base.py:2067 — not affected: default
handle_cursor is a no-op.
- superset/db_engine_specs/hive.py:200 — not affected: PyHive fetching has
no signal-driven cancel follow-up on the interrupted transport.
- superset/db_engine_specs/hive.py:436 — not affected: explicit Stop calls
cancel after poll returns; the TIME_LIMIT_EXCEPTIONS handler at line 465
re-raises without a cancellation RPC.
- superset/db_engine_specs/spark.py:51 — not affected: inherits Hive
handling, which re-raises time limits without following them with cancellation
on the interrupted transport.
- superset/sql_lab.py:375 — not affected: marks the query timed out and
raises an application error; it does not send a cancellation RPC on the
interrupted transport.
- superset/db_engine_specs/crate.py:99 — not affected: post-processes
generic fetch results without a time-limit cancel handler.
- superset/db_engine_specs/exasol.py:165 — not affected: converts generic
fetch rows without a time-limit cancel handler.
- superset/db_engine_specs/elasticsearch.py:274 — not affected: HTTP cursor
pagination, not impyla signal-driven cancellation.
- superset/db_engine_specs/elasticsearch.py:392 — not affected: OpenDistro
HTTP cursor pagination, not impyla signal-driven cancellation.
- superset/db_engine_specs/mssql.py:194 — not affected: converts ODBC rows
without a time-limit cancel handler.
- superset/db_engine_specs/bigquery.py:416 — not affected: progressive
fetching has no cancellation RPC after an interrupted reply.
- superset/db_engine_specs/datastore.py:319 — not affected: converts fetched
rows without a time-limit cancel handler.
- superset/db_engine_specs/ocient.py:322 — not affected: fetch failure
removes an in-memory mapping and re-raises, without cancellation.
- superset/db_engine_specs/ocient.py:383 — not affected: records the query
identifier then delegates to the no-op base handle_cursor.
- superset/db_engine_specs/drill.py:187 — not affected: handles an
empty-result RuntimeError without issuing cancellation.
- superset/db_engine_specs/trino.py:331 — not affected: Trino cursor
handling does not use impyla or cancel in a signal-exception handler.
- superset/db_engine_specs/presto.py:1415 — not affected: explicit Stop
cancellation follows a completed poll, not an interrupted reply.
--
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]
