EnxDev commented on code in PR #44737:
URL: https://github.com/apache/superset/pull/44737#discussion_r4137546507
##########
superset/db_engine_specs/impala.py:
##########
@@ -134,6 +129,7 @@ def handle_cursor(cls, cursor: Any, query: Query) -> None:
query_id = query.id
unfinished_states = (
+ "PENDING_STATE",
Review Comment:
With PENDING in the loop, a query whose first `status()` is PENDING now
sleeps the full `DB_POLL_INTERVAL_SECONDS` (5s by default) before the next
check. Before, it left the loop and `fetch_data` picked it up through impyla's
sub-second `_wait_to_finish` polling.
Might be misreading how often a fresh query is still in admission at that
first poll. If it's common, every short Impala query in SQL Lab gains up to 5s.
Would a short first sleep while PENDING keep the cancel window without that
cost?
##########
superset/db_engine_specs/impala.py:
##########
@@ -105,14 +107,7 @@ def get_schema_names(cls, inspector: Inspector) ->
set[str]:
@classmethod
def has_implicit_cancel(cls) -> bool:
- """
- Return True if the live cursor handles the implicit cancelation of the
query,
- False otherwise.
-
- :return: Whether the live cursor implicitly cancels the query
- :see: handle_cursor
- """
-
+ """Keep HTTP cancellation independent of the live cursor polling
loop."""
Review Comment:
The new STOPPED check only runs if `cancel_query()` returned True, and for
Impala that needs a `QUERY_CANCEL_KEY` captured before `execute_async`. With no
database in the URI, impyla's `cursor()` never runs `USE`, so the key is None.
`QUERY_DISPATCHED_KEY` is set, so Stop raises "Could not cancel query" and the
row never reaches STOPPED. With a database, the stored ID belongs to the
already-finished `USE` operation.
Could `prepare_cancel_query()` set `QUERY_EARLY_CANCEL_KEY` when there's no
cancel key, the way `trino.py:486` does? Both cases would then land on the
live-cursor cancel. A test with a dispatched query and no cancel key would
cover it.
##########
superset/db_engine_specs/impala.py:
##########
@@ -147,33 +143,37 @@ def handle_cursor(cls, cursor: Any, query: Query) -> None:
# the query was still executed
# modified in stop_query in views / core.py is reflected here.
# stop query
- if query.extra.get(QUERY_EARLY_CANCEL_KEY):
- cursor.cancel_operation()
- cursor.close_operation()
- cursor.close()
+ if query.extra.get(QUERY_EARLY_CANCEL_KEY) or query.status in (
+ QueryStatus.STOPPED,
+ QueryStatus.TIMED_OUT,
Review Comment:
Nothing persists TIMED_OUT while this loop is running. Both writers
(`sql_lab.py:358`, `celery_task.py:467`) are this same worker's
`SoftTimeLimitExceeded` handlers. That exception subclasses `Exception`, so if
it fires during the poll the `except` at line 182 swallows it. `fetch_data`
then waits for the query until the hard limit kills the worker.
Mind catching `SoftTimeLimitExceeded` there, cancelling the operation and
re-raising? The `timed_out` test only passes because it sets the status on the
mock directly.
--
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]
