aminghadersohi commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4431913416
##########
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:
I missed the proxy `_closed` and cleanup-RPC cases in my earlier replies.
This follow-up is in 2e1b2559617d4e5089b4d88bf1405f1ed972a0bd.
- `_LimitedCursor` forwards driver state writes to the real cursor,
including nested wrappers and the truncation probe. Abandonment clears the
operation handle and description as well as setting `_closed`, so cursor
close/finalization and later metadata/status/log reads send no further
operation RPCs.
- A second time limit or an unread/invalid reply during CancelOperation,
CloseOperation or CloseSession abandons the cursor and drops/resets the
transport instead of continuing cleanup. Cleanup failures are logged without
replacing the original time-limit exception. Stop/deadline cleanup uses the
same abandonment rule. A fully consumed error-status reply can still release
the handles.
- Hive polling/fetching also abandons interrupted operations. PyHive
connection close is made local-only before dropping the socket, so even an
interrupted local drop cannot cause a later CloseSession RPC on stale bytes.
Regression evidence (real buffered Thrift transports and locally served
HiveServer2 replies, not just mocked RPC order):
- Before, at 365c5e877d6cb893da2e2b590bed770a4c2c4c41: `pytest -q
tests/unit_tests/db_engine_specs/test_impala_transport.py -k 'row_limited or
time_limit_during_cleanup'` → **48 failed, 37 deselected**. Failures included
`TTransportException: read timeout` while closing the real cursor after an
interrupted reply.
- Before: `pytest -q
tests/unit_tests/db_engine_specs/test_hive_transport.py` → **12 failed**.
Separately, `-k test_connection_close_sends_no_rpc_after_an_interrupted_reply`
→ **4 failed, 24 deselected**, with the traceback inside PyHive
`Connection.close` → `recv_CloseSession`, reading stale bytes.
- After, at 2e1b2559617d4e5089b4d88bf1405f1ed972a0bd:
`test_impala_transport.py` → **175 passed**; `test_hive_transport.py` → **28
passed**. The expanded cases include
`test_row_limited_fetch_sends_nothing_after_a_skipped_cancel`,
`test_time_limit_during_cleanup_abandons_the_cursor`,
`test_failed_stop_cleanup_rpc_abandons_the_cursor`,
`test_cleanup_sends_nothing_after_an_interrupted_reply`,
`test_connection_close_sends_no_rpc_after_an_interrupted_reply`, and
`test_failed_transport_drop_preserves_timeout_and_disables_session_close`. They
exercise both SoftTimeLimitExceeded and SupersetTimeoutException,
status/log/row interruptions, binary/SASL PLAIN/HTTP for impyla, and
binary/SASL PLAIN for PyHive. The original exception's identity and the last
RPC received are asserted.
Compatibility suites at the same commit: `test_impala.py` 61 passed;
`test_impala_pending.py` 42 passed; `test_hive.py` 29 passed; `test_base.py`
147 passed; `test_presto.py` 135 passed; `test_executor.py` 280 passed. Total:
897 passed.
All pytest commands used `PYTHONPATH=$PWD/superset-core/src:$PWD` and
deselected `manual` and `llm_compliance`. Pre-commit, including mypy, passed on
the staged changes; branch-file checks also passed.
Siblings checked:
- superset/db_engine_specs/impala.py:140,167,203,344,370 — changed: fetch,
poll/status/log, deadline and Stop cleanup abandon interrupted operations; no
retry after a cleanup limit. Real transport tests cover CancelOperation,
CloseOperation and CloseSession. Abandonment clears the real operation handle
and cached description so metadata/status/log calls cannot use it afterwards.
- superset/sql/execution/executor.py:208,231,385,922,1648 — changed:
row-limit and truncation wrappers forward driver writes to the real cursor; the
enclosing closing(real_cursor) therefore sees abandonment. Nested wrapper
regression plus real row/probe interrupted replies.
- superset/db_engine_specs/hive.py:200,220,460,469 — changed: poll/log/row
time limits clear the operation and replace PyHive connection.close with local
transport close before dropping the socket. Tests isolate Connection.close as
well as cursor close, including a failed or interrupted local drop.
- superset/sql_lab.py:369,375,905 — not affected independently: delegates
fetch/execute/poll to the changed specs, wraps the resulting time limit, and
its cancellation fallback opens a fresh connection, not the interrupted cursor
transport. Cursor cleanup is exercised through closing(real_cursor) in the
transport regressions.
- superset/models/core.py:725,837 — not affected independently:
get_raw_connection defaults to NullPool and closes the DB-API connection; the
changed Hive helper makes that close local-only, and impyla HS2Service.close
(installed impala/hiveserver2.py:1272) closes only its transport.
- superset/db_engine_specs/base.py:144,1624,2067,2101 — not affected
independently: defines the two time-limit classes; base fetch only
reads/converts rows and maps exceptions, and base handle_cursor is a no-op. The
driver wrappers reach the changed spec handlers; Base tests pass.
- superset/db_engine_specs/presto.py:1400,1415 — not affected: Presto uses
HTTP JSON polling and cursor.cancel, not HiveServer2 Thrift reply buffers or
impyla cleanup; its fetch caller still dispatches through cls.fetch_data.
Presto tests pass.
- superset/db_engine_specs/trino.py:331,410,500 — not affected: Trino cursor
polling/cancellation uses the Trino HTTP client, not the impyla/PyHive
HiveServer2 transport or a cursor proxy.
- superset/db_engine_specs/ocient.py:322,383 — not affected: pyocient row
conversion and query-ID mapping; no HiveServer2 cleanup or attribute proxy.
- superset/db_engine_specs/exasol.py:165 — not affected: only converts rows
returned by Base.fetch_data; no cleanup RPC/proxy.
- superset/db_engine_specs/mssql.py:194 — not affected: only converts pyodbc
rows returned by Base.fetch_data; no cleanup RPC/proxy.
- superset/db_engine_specs/crate.py:99 — not affected: converts timestamp
values from CrateDB HTTP results; no HiveServer2 transport/cleanup.
- superset/db_engine_specs/datastore.py:319 — not affected: converts Google
Datastore row values; no HiveServer2 transport/cleanup.
- superset/db_engine_specs/drill.py:187 — not affected: special-cases a
RuntimeError for empty Drill results; no HiveServer2 cleanup or proxy.
- superset/db_engine_specs/bigquery.py:416,509 — not affected: BigQuery
progressive row fetching and Base fallback; no impyla/PyHive cleanup or proxy.
- superset/db_engine_specs/elasticsearch.py:70,146,274,392 — not affected:
Elasticsearch/OpenDistro close uses an HTTP perform_request with a pagination
token, not a Thrift cursor/transport/proxy.
- superset/db_engine_specs/postgres.py:115 — not affected: psycopg
connection-validation cursor close after SELECT version(); not HiveServer2.
- superset/db_engine_specs/redshift.py:44,430 — not affected: one match is a
release comment; executable close follows PostgreSQL pg_cancel_backend SQL, not
HiveServer2.
- superset/db_engine_specs/databend.py:424 — not affected: module-level
__getattr__ is a backwards-compatible engine-class alias, not a cursor-state
proxy. No other cursor proxy exists in db_engine_specs (rg
class/attribute-delegation search).
- superset/db_engine_specs/clickhouse.py:722,837,936 — not affected:
port-error text, inherited Base.handle_cursor, and a race-condition comment; no
HiveServer2 reply/cleanup path.
- superset/db_engine_specs/mysql.py:156,158,164,172,180,211 — not affected:
fail-closed TLS policy names/comments, not cursor close/reconnect.
- superset/db_engine_specs/doris.py:149 — not affected: TLS fail-closed
comment, not cursor close.
- superset/db_engine_specs/duckdb.py:48 — not affected: close=y in an
authentication URL, not cursor cleanup.
- superset/db_engine_specs/databricks.py:722 — not affected: closed-port
error message, not cursor cleanup.
- superset/db_engine_specs/couchbase.py:271 — not affected: closed-port
error message, not cursor cleanup.
Remaining: real Impala server acceptance, coordinator admission-slot/session
release, and LDAP/GSSAPI security-layer negotiation were not rerun here. When
cleanup is abandoned, server-side handles remain subject to Impala/Hive
query/session timeouts; these tests do not establish server-side release.
--
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]
