aminghadersohi commented on code in PR #45232:
URL: https://github.com/apache/superset/pull/45232#discussion_r4278161301
##########
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 the "Remaining" note in my previous reply: this PR now does
change celery_task._handle_query_error. Commit
758e87f60fa44f24e8d7297aa61a4cfb3d4bf28c keeps a committed STOPPED when the
worker's error handler runs after the cancel
(tests/unit_tests/sql/test_async_cancel.py::test_worker_error_after_committed_stop_keeps_stopped
fails without it), so the async cancel chain ends STOPPED with this PR alone.
https://github.com/apache/superset/pull/45283 changes the same function; the
two will be aligned before either merges.
Remaining: the success write and the results-backend FAILED write in
celery_task.py can still overwrite a STOPPED committed after the worker's last
stop check. That finish-versus-stop race is the same on master and is handled
in https://github.com/apache/superset/pull/45283.
--
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]
