EnxDev commented on code in PR #45283:
URL: https://github.com/apache/superset/pull/45283#discussion_r4281107033
##########
superset/sql/execution/executor.py:
##########
@@ -1427,15 +1428,22 @@ def _get_async_query_result(query_id: int) -> Any:
)
@staticmethod
+ @transaction()
def _cancel_async_query(query_id: int, database: Database) -> bool:
- """Cancel an async query."""
+ """Cancel an async query and persist intent without replacing a
result."""
from superset.models.sql_lab import Query as QueryModel
+ from superset.sql.execution.utils import set_query_stopped
+ from superset.utils.dates import now_as_float
query =
db.session.query(QueryModel).filter_by(id=query_id).one_or_none()
if not query:
return False
- return SQLExecutor._cancel_query(database, query)
+ with db.session.no_autoflush:
+ if not SQLExecutor._cancel_query(database, query):
+ return False
+ set_query_stopped(query, now_as_float())
+ return True
Review Comment:
`set_query_stopped` reports when a finished result won the race, but its
return value is dropped here. So `AsyncQueryHandle.cancel()` says True for a
query that ended SUCCESS, FAILED or TIMED_OUT, and an extension that trusts it
will treat a completed query as cancelled.
Passing it through fixes that.
`test_engine_early_cancel_preserves_concurrently_committed_terminal_metadata`
(`tests/unit_tests/daos/test_query_dao.py:157`) currently asserts `is True` in
exactly that case and would flip to `is False`.
```suggestion
return set_query_stopped(query, now_as_float())
```
--
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]
