aminghadersohi commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4423654792
##########
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:
+ cursor.cancel_operation()
+ raise TimeoutError(
+ "Timed out waiting for the Impala operation"
+ )
Review Comment:
Fixed in d66573316f59337fe1282d0e14d4d28349c67a70. The deadline uses the
existing cancel-and-release helper, so a driver cancellation error is logged,
both operation/cursor handles are closed, and TimeoutError propagates.
Regression: `test_fetch_data_deadline_releases_operation_when_cancel_fails`
in `tests/unit_tests/db_engine_specs/test_impala.py` injects
`impala.error.Error` through the existing optional-driver fixture. Before the
change, `PYTHONPATH=.:superset-core/src pytest -q
tests/unit_tests/db_engine_specs/test_impala.py -k deadline_releases -m 'not
manual and not llm_compliance'` reported `1 failed, 26 deselected` with the
cancellation driver error replacing TimeoutError. After the change, the three
Impala unit files reported `67 passed`; `pre-commit run --files
superset/db_engine_specs/impala.py
tests/unit_tests/db_engine_specs/test_impala.py` passed all applicable hooks.
Siblings checked:
- superset/db_engine_specs/impala.py:150 — fixed in
d66573316f59337fe1282d0e14d4d28349c67a70
- superset/db_engine_specs/impala.py:159 — not affected: the sole `except
Error` maps driver failures, not time-limit exceptions; deadline cancellation
errors are contained by the helper before reaching it.
- superset/db_engine_specs/impala.py:166 — not affected: the outer
time-limit handler catches ordinary cancel failures and re-raises the time
limit without driver-error mapping.
- superset/db_engine_specs/impala.py:177 — not affected: the helper catches
ordinary cancel failures, continues through close_operation and close, and
propagates time-limit exceptions separately.
- superset/db_engine_specs/impala.py:228 — not affected: explicit Stop
already uses the cancel-and-release helper.
- superset/db_engine_specs/impala.py:259 — not affected: handle_cursor
time-limit cleanup already uses the cancel-and-release helper.
Search: `rg -n 'cancel_operation\(|except Error'
superset/db_engine_specs/impala.py`; the other matches are warning strings and
documentation, not RPC calls or handlers. No containers started; server-side
release was not remeasured.
--
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]
