aminghadersohi commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4432706274
##########
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 replaced the per-RPC reconnect/cancel approach in
bb609de6337fcdbff1bd5aa74301782a61b06a19 with a shared fail-closed HiveServer2
client guard. It abandons the real cursor and connection in the exception path,
before driver handlers can send another RPC. This includes ExecuteStatement,
lazy GetResultSetMetadata, status/log/row reads and cleanup. It also blocks
impyla's transport reopen *before* its driver-internal CloseSession lookup.
Cleanup failures are logged without replacing the original limit; no
cancellation is attempted after an interrupted reply.
Regression tests include
`test_execute_and_metadata_abandon_before_driver_cleanup`,
`test_execute_and_metadata_abandon_before_cursor_and_connection_cleanup`,
`test_cleanup_rpc_timeout_abandons_before_driver_handlers`,
`test_second_limit_during_local_drop_preserves_execute_timeout`,
`test_driver_close_timeout_prevents_transport_reopen`, and
`test_sqlalchemy_ping_abandons_before_dialect_cursor_cleanup`. The fixtures use
real Thrift transports and raise after reading the reply header, leaving the
body unread. Impala and PyHive binary, SASL PLAIN and HTTP paths are exercised,
plus actual SIGALRM and the Windows timer's pre-conversion KeyboardInterrupt.
Before counts measured in an isolated worktree at the starting head,
2e1b2559617d4e5089b4d88bf1405f1ed972a0bd, with the final regression assertions
retained:
| Test file | Before | After |
| --- | --- | --- |
| `test_impala.py` | 37 failed, 24 passed | 61 passed |
| `test_impala_pending.py` | 7 failed, 35 passed | 42 passed |
| `test_impala_transport.py` | 192 failed, 49 passed (partitioned runs
below) | 241 passed |
| `test_hive_transport.py` | 28 failed, 32 passed | 60 passed |
The Impala transport before count is a completed 235-case run (186 failed,
49 passed) plus the six direct-driver-close cases (six failed). I used
`--assert=plain --tb=no` for the completed broad before runs because pytest's
failure rendering hit an internal AST error in earlier attempts; those
incomplete attempts are not the counts above. These are measurements from this
run, not the before counts in earlier replies.
Commands: `PYTHONPATH=.:superset-core/src pytest -q --assert=plain --tb=no
tests/unit_tests/db_engine_specs/<file> -m 'not manual and not
llm_compliance'`; the added six-case before subset used `-k
driver_close_timeout`. The corrected verifier probe also went from 16 failed to
16 passed; its Hive assertion selects the interrupted ExecuteStatement rather
than the setup USE statement.
At the pushed head, additional narrow runs passed: Hive 29, Base 147, Presto
135, SQL/execution 771, legacy SQL Lab 92, and model connection/prequery checks
10 (1,588 committed-suite cases total). SQLite-heavy final reruns used pytest
`--basetemp` under tmpfs to avoid host journal I/O stalls; no assertions or
driver mocks were changed for that rerun. Pre-commit, including mypy, passed on
the branch's changed files.
Siblings checked:
- superset/utils/hive.py:37 — fixed in
bb609de6337fcdbff1bd5aa74301782a61b06a19; one generated-client guard,
real-cursor registration, and rejection of later RPC/open attempts.
- superset/db_engine_specs/base.py:143,2097 — fixed in
bb609de6337fcdbff1bd5aa74301782a61b06a19; shared cursor scope, including
Windows KeyboardInterrupt before timeout conversion.
- superset/db_engine_specs/impala.py:137,153,174,189,206,401 — fixed in
bb609de6337fcdbff1bd5aa74301782a61b06a19; execution, metadata, polling, fetch,
cleanup and driver-internal reopen all use the same abandonment state.
- superset/db_engine_specs/hive.py:210,226,232,240,493,702 — fixed in
bb609de6337fcdbff1bd5aa74301782a61b06a19; PyHive's shared client and
connection/cursor cleanup, with binary, SASL and HTTP regressions.
- superset/models/core.py:674,684,816,858 — fixed in
bb609de6337fcdbff1bd5aa74301782a61b06a19; connection preparation, prequeries,
and per-engine event installation before engine caching.
- superset/sql/execution/executor.py:197,237,368,649,927,1653 — fixed in
bb609de6337fcdbff1bd5aa74301782a61b06a19; real cursor scope includes lazy
description and row-limit probes; probes propagate limits rather than reporting
partial success.
- superset/sql_lab.py:216,324,337,415,915 — fixed in
bb609de6337fcdbff1bd5aa74301782a61b06a19; legacy execution scope includes
description and prepares fallback cancellation connections.
- superset/sql/execution/celery_task.py:544,622,626 — not affected:
unchanged caller obtains prepared connections/cursors and delegates to the
guarded specs; its 241 unit cases pass.
- superset/db_engine_specs/ascend.py:24 — not affected: no local override of
the six guarded Impala methods; method identities were checked against the
parent.
- superset/db_engine_specs/spark.py:44 — not affected: inherits all six
guarded Hive methods unchanged; method identities checked.
- superset/db_engine_specs/databricks.py:1166 — not affected: the PyHive
descendant inherits the six guarded Hive methods unchanged; the native
connector is separate.
- superset/db_engine_specs/presto.py:1415 — not affected: JSON/HTTP polling
and cursor.cancel, not a HiveServer2 Thrift transport; 135 tests pass.
- superset/db_engine_specs/trino.py:331 — not affected: Trino HTTP client
and execute-thread state, not impyla/PyHive HiveServer2 cleanup.
- superset/db_engine_specs/kusto.py:348 — not affected: transforms KQL text
then delegates to Base; no HiveServer2 client or cursor proxy.
- superset/db_engine_specs/d1.py:94 — not affected: moves SQL comments for
the D1 driver; no HiveServer2 client or cursor proxy.
- superset/db_engine_specs/clickhouse.py:840 — not affected: ClickHouse
execution settings, not a HiveServer2 cursor/transport.
- superset/db_engine_specs/bigquery.py:416 — not affected: BigQuery
progressive row reads; no impyla/PyHive cleanup or attribute proxy.
- superset/db_engine_specs/mssql.py:194 — not affected: pyodbc row
conversion after Base.fetch_data, without HiveServer2 cleanup.
- superset/db_engine_specs/crate.py:99 — not affected: converts CrateDB
result timestamps; no HiveServer2 cleanup.
- superset/db_engine_specs/ocient.py:322,383 — not affected: pyocient result
conversion and query-ID mapping, not HiveServer2 cleanup or an attribute proxy.
- superset/db_engine_specs/exasol.py:165 — not affected: row conversion
after Base.fetch_data; no HiveServer2 transport.
- superset/db_engine_specs/drill.py:187 — not affected: special-cases an
empty Drill result error; no HiveServer2 transport.
- superset/db_engine_specs/datastore.py:319 — not affected: converts Google
Datastore rows; no HiveServer2 transport.
- superset/db_engine_specs/databend.py:424 — not affected: module-level
compatibility alias, not a cursor wrapper.
- superset/utils/core.py:815,854 — not affected: timeout implementations
unchanged; actual SIGALRM and pre-conversion timer exceptions are covered by
the transport tests.
- superset/sqllab/sql_json_executer.py:128 — not affected: unchanged timeout
owner delegates to the legacy execution scope above.
- superset/commands/database/utils.py:188 — not affected: no local change;
the per-engine connection hook prepares dialect ping cursors before use,
exercised by both real-transport ping tests.
- superset/commands/sql_lab/estimate.py:197 — not affected: no local
transport handling; Base.estimate_query_cost obtains a prepared raw connection
and guarded cursor factory.
- superset/commands/database/validate_sql.py:105,175 — not affected: no
shipped Hive/Impala SQL validator in the default registry; only Presto is
enabled there.
Searches: `rg -n 'utils\.timeout|\btime_limit\b|soft_time_limit|class
.*Cursor|__getattr__|__setattr__|closing\(|def
(execute|fetch_data|handle_cursor)\(' superset/db_engine_specs superset/sql
superset/sql_lab.py`, plus the repository-wide timeout-owner and Hive/Impala
descendant searches. Only paths marked with a commit above have a
production-file diff in this round.
Remaining: real Impala was not run; server-side operation/admission-slot
release and LDAP/GSSAPI negotiation are not proven. Abandoned operations rely
on server query/session timeouts. The pre-existing lack of SIGALRM enforcement
in threaded synchronous execution and the hardcoded Celery task-limit
configuration remain outside this change. I am leaving this thread unresolved.
--
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]
