aminghadersohi commented on code in PR #45232:
URL: https://github.com/apache/superset/pull/45232#discussion_r4276297271
##########
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:
Correction to my previous reply: one Trino path was still not fixed. A Trino
stop issued before the query ID is published commits only the early-cancel
flag. The worker could see the flag, kill the statement and commit FAILED
before `stop_query` read the row back, so Stop returned success and the query
stayed FAILED.
Fixed in 4a2cc30492ddd960bca72388122c119fd9a914eb: Trino's `handle_cursor`
records STOPPED once its kill succeeds, before the killed statement raises, so
`handle_query_error` keeps it. A failed kill records nothing, and TIMED_OUT is
not changed.
Regression tests (fail at 8d0fcf8ab3, pass at 4a2cc30492):
-
`tests/unit_tests/dao/query_stop_isolation_test.py::test_trino_early_cancel_that_kills_the_worker_ends_stopped`
(real DAO, real Trino `execute_with_cursor`/`handle_cursor`, real
`handle_query_error`, worker in a second SQLite session; ended `failed` before)
-
`tests/unit_tests/db_engine_specs/test_trino.py::test_handle_cursor_early_cancel_records_stop_only_when_killed[running-True-stopped]`
Controls that pass before and after:
`test_trino_early_cancel_keeps_failure_that_predates_it` (a FAILED committed
before the flag stays FAILED with its error), and the `[running-False-running]`
(kill failed, no STOPPED) and `[timed_out-True-timed_out]` cases.
Siblings checked:
- superset/db_engine_specs/trino.py:406 — fixed in
4a2cc30492ddd960bca72388122c119fd9a914eb
- superset/db_engine_specs/presto.py:1431 — not affected: implicit cancel;
the worker cancels only after it reads a committed STOPPED, and
`handle_query_error` keeps STOPPED
- superset/db_engine_specs/hive.py:448 — not affected: same implicit cancel
on a committed STOPPED; covers Spark and DatabricksHive, which subclass Hive
- superset/db_engine_specs/impala.py:244 — not affected:
`prepare_cancel_query` commits STOPPED with the flag before the worker can act;
covers Ascend, which subclasses Impala
- superset/db_engine_specs/databricks.py:255 — not affected: no cancel
overrides, so it takes the generic no-ID path below
- superset/sql_lab.py:937 — not affected: the generic no-ID path stages the
flag uncommitted and `stop_query` commits it together with STOPPED, so the
worker never sees the flag alone
- superset/daos/query.py:109 — not affected: Postgres, MySQL, Snowflake,
Redshift, Oracle, SingleStore, Ocient and Trino with a published ID kill from
outside the worker, and the conditional write from the earlier commit replaces
that FAILED (`test_stop_query_replaces_failure_caused_by_its_cancel`)
Remaining: the async executor chain (`AsyncQueryHandle.cancel` ->
`SQLExecutor._cancel_async_query` -> `_cancel_query`) still returns success
without writing STOPPED, and its worker error handler
`celery_task._handle_query_error` writes FAILED over STOPPED. So an external
kill, or a Trino early-flag kill, through that executor still ends FAILED. This
is the same on master and is not fixed in this PR.
--
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]
