aminghadersohi commented on code in PR #45232:
URL: https://github.com/apache/superset/pull/45232#discussion_r4275787664
##########
superset/daos/query.py:
##########
@@ -70,26 +70,50 @@ def stop_query(client_id: str) -> None:
if not query:
raise QueryNotFoundException(f"Query with client_id {client_id}
not found")
- if query.status in [
- QueryStatus.FAILED,
- QueryStatus.SUCCESS,
- QueryStatus.TIMED_OUT,
- ]:
+ if query.status in FINISHED_QUERY_STATUSES:
logger.warning(
"Query with client_id could not be stopped: query already
complete",
)
return
- if not sql_lab.cancel_query(query):
+ cancelled = sql_lab.cancel_query(query)
+ if query.status in FINISHED_QUERY_STATUSES:
+ # Preparing the stop can observe the worker's terminal commit.
+ logger.warning(
+ "Query with client_id could not be stopped: query already
complete",
+ )
+ return
+ if not cancelled:
raise SupersetCancelQueryException("Could not cancel query")
+ if query.status == QueryStatus.STOPPED:
+ # The engine spec already committed the stop while preparing it
+ # (see ImpalaEngineSpec.prepare_cancel_query).
+ return
+
# cancel_query() may have staged an early-cancel flag on query.extra
# without committing it (see its docstring/comments); the
# @transaction decorator commits it together with status=STOPPED
# below in one transaction, closing the window where another
# request could observe the flag set but the status still RUNNING.
- query.status = QueryStatus.STOPPED
- query.end_time = now_as_float()
+ #
+ # The write is conditional: the worker can commit a terminal status
+ # after the check above, and that result must not be overwritten.
+ stopped = (
+ db.session.query(Query)
+ .filter(
+ Query.id == query.id,
+ Query.status.notin_(FINISHED_QUERY_STATUSES),
Review Comment:
Agreed, the conditional write was too broad. Fixed in
8d0fcf8ab3f09d29b2b21afbd120cac688f18c19: `QueryDAO.stop_query` now skips the
STOPPED write only for SUCCESS and TIMED_OUT, so a FAILED committed by the
worker after the cancel killed its statement is replaced with STOPPED again, as
on master.
Regression test, added to
`tests/unit_tests/dao/query_stop_isolation_test.py` as you suggested:
`test_stop_query_replaces_failure_caused_by_its_cancel`. The worker commits
FAILED ("terminating connection due to administrator command") inside
`cancel_query`, on the real SQLite harness. At 70fec12f it fails with
`AssertionError: assert 'failed' == 'stopped'`; at 8d0fcf8ab3 it passes.
`test_stop_query_keeps_outcome_a_stop_cannot_cause[success|timed_out]` covers
the statuses that stay protected.
Siblings checked:
- superset/daos/query.py:109 — fixed in
8d0fcf8ab3f09d29b2b21afbd120cac688f18c19
- superset/daos/query.py:80 — not affected: for engines that kill from
outside, status is only in-memory there and nothing refreshes it after the kill
- superset/db_engine_specs/impala.py:283 — not affected: the claim runs
before any cancel reaches Impala, so a FAILED it skips was not caused by the
stop
- superset/sql_lab.py:891 — not affected: Trino's `prepare_cancel_query`
commit runs before the kill, so only a FAILED that predates the stop is seen
- superset/sql/execution/executor.py:1465 — not affected: same reason as
`sql_lab.py:891`
- superset/sql_lab.py:136 — not affected: `handle_query_error` reloads
status and keeps a committed STOPPED
- superset/sql_lab.py:828 — not affected: the SUCCESS write keeps FAILED and
STOPPED
- superset/db_engine_specs/hive.py:447 — not affected: cancels from the
worker and writes no status (Presto `presto.py:1430` and Trino `trino.py:375`
are the same)
Remaining (same on master, not changed by this PR): a stop through
`SQLExecutor._cancel_query` (`superset/sql/execution/executor.py:1441`) never
writes STOPPED, and `celery_task._handle_query_error`
(`superset/sql/execution/celery_task.py:88`) writes FAILED, so on an engine
that kills from outside that path can end FAILED. Nothing in the repo calls
`AsyncQueryHandle.cancel()`; I left it out of this PR.
Tests: `query_stop_isolation_test.py`, `queries_test.py`,
`test_impala_pending.py`, `test_executor.py`, `test_celery_task.py`,
`sql_lab_test.py`: 395 passed. The only failures are 8
`test_apply_limit_generate_series_returned_rows` cases that need `duckdb`,
which isn't installed locally; they fail the same way at 70fec12f.
--
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]
