aminghadersohi commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4430548326
##########
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:
Follow-up to my reply above: it said no cancel RPC is sent when the
transport reset is skipped, which was true for the cancel but not for cleanup.
On that path impyla's `cursor.close()` (the executor reaches it through
`closing(conn.cursor())`) and the cursor finalizer still sent CloseOperation
and CloseSession, reopening the transport with the interrupted reply still
buffered.
Fixed in 365c5e877d6cb893da2e2b590bed770a4c2c4c41:
`_cancel_operation_after_time_limit` marks the cursor closed
(`_abandon_cursor`) when the reset returns False or raises, and when a second
time limit lands during the reset, so impyla's `close()` and finalizer send
nothing.
Regression test:
`tests/unit_tests/db_engine_specs/test_impala_transport.py::test_closing_the_cursor_sends_nothing_after_a_skipped_cancel`
[binary, sasl, http] x [unknown_transport, reset_raises]. It runs
`handle_cursor` inside `closing(cursor)` over the real impyla transports,
interrupts the first status reply, then calls the finalizer, and asserts the
server received no request after the interrupted one. Without `_abandon_cursor`
all 6 fail (`TTransportException: read timeout` on binary and SASL, `EOFError`
over HTTP, and that error replaces the time-limit exception); with it they
pass. Mock tests: `test_time_limit_skips_cancel_rpcs_without_a_clean_transport`
now also asserts the cursor is marked closed, and
`test_time_limit_during_transport_reset_propagates` covers a second
SoftTimeLimitExceeded or SupersetTimeoutException during the reset.
test_impala, test_impala_pending, test_impala_transport, test_hive,
test_presto, test_trino: 473 passed.
Siblings checked:
- superset/db_engine_specs/impala.py:180 — fixed in
365c5e877d6cb893da2e2b590bed770a4c2c4c41 (second time limit during the reset)
- superset/db_engine_specs/impala.py:190 — fixed in
365c5e877d6cb893da2e2b590bed770a4c2c4c41 (reset returned False or raised)
- superset/sql/execution/executor.py:920 — not affected: closes the cursor
through `closing()`, which is now a no-op for an abandoned cursor
- superset/sql/execution/executor.py:1647 — not affected: same `closing()`
pattern, a no-op for an abandoned cursor
- superset/sql_lab.py:593 — not affected: the cursor is not closed
explicitly; its finalizer is a no-op for an abandoned cursor and the connection
close only closes the transport
Remaining: `requirements/development.txt` does not install impyla or thrift,
so CI skips `test_impala_transport.py` through `pytest.importorskip`; I ran it
locally with impyla 0.24.0, thrift 0.24.0 and thrift_sasl 0.4.3. The items
listed under Remaining in my reply above still apply.
--
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]
