rebenitez1802 commented on code in PR #45232:
URL: https://github.com/apache/superset/pull/45232#discussion_r4274734056
##########
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:
**🔴 High — Conditional STOPPED write turns a stop into FAILED on non-Impala
engines**
This skips the STOPPED write when the row is in `FINISHED_QUERY_STATUSES`,
and that set includes FAILED. Engines whose cancel kills the statement from
outside the worker are affected: Postgres (`pg_terminate_backend`), MySQL
(`KILL CONNECTION`), Snowflake, Redshift, Oracle, and Trino once the query id
is published.
- The worker's statement fails straight away, so its error handler can
commit FAILED before this UPDATE runs.
- The UPDATE then matches no row. The Stop request returns 200, but the
query ends `failed` with the driver's "terminated" error in Query History and
the API.
- Master's unconditional write ended STOPPED whichever side committed first.
We reproduced it with the
`test_stop_query_keeps_status_committed_after_its_check` harness by having the
worker commit FAILED inside the cancel call: the query ended `failed` at head
and `stopped` with master's `daos/query.py`. Impala never reaches this write;
it returns early after its own claim at `:89`.
This came from my round-1 suggestion to make the write conditional; I should
have said only statuses a stop can't cause should be protected. Suggested fix
below. Please also add a FAILED case to `query_stop_isolation_test.py`, where
the worker commits FAILED during `cancel_query`. Alternatively, move this
conditional write to #45283 and keep only the Impala early return here.
```suggestion
# The write is conditional: the worker can commit SUCCESS or
TIMED_OUT
# after the check above, and that result must not be overwritten. A
# FAILED committed in this window is usually caused by the cancel
# itself, so it is still replaced with STOPPED.
stopped = (
db.session.query(Query)
.filter(
Query.id == query.id,
Query.status.notin_((QueryStatus.SUCCESS,
QueryStatus.TIMED_OUT)),
```
--
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]
