aminghadersohi commented on PR #45232: URL: https://github.com/apache/superset/pull/45232#issuecomment-6151148844
AI review at ab5df9b3 (full PR diff vs master), fixed in 386b5e0b. **MEDIUM: a stop that loses the race with completion still relabels the finished query as STOPPED.** When `ImpalaEngineSpec.prepare_cancel_query` declines to claim the stop (the worker has already committed SUCCESS/FAILED/TIMED_OUT), it refreshes the query and returns. But `sql_lab.cancel_query` then carried on to the coordinator HTTP cancel for the finished operation (`QUERY_CANCEL_KEY` is set by then). If that returned True, `QueryDAO.stop_query` wrote STOPPED and a new `end_time` over the terminal row. That is the same overwrite ab5df9b3 fixed inside `prepare_cancel_query`, now one layer up, and it also reintroduced the HTTP cancel this PR removes. `SQLExecutor._cancel_query` had the same fall-through, though it does not write a status, so there it only sent a spurious HTTP cancel. Fix (386b5e0b): after `prepare_cancel_query`, `cancel_query` returns without cancelling if the query is finished, `stop_query` re-checks and keeps the terminal status, and `SQLExecutor._cancel_query` returns False. Regression tests: `tests/unit_tests/dao/queries_test.py::test_query_dao_stop_query_keeps_status_when_impala_finishes_first` (runs the real Impala `prepare_cancel_query` against sqlite, with the worker's terminal commit landing between the DAO read and the claim) and `tests/unit_tests/sql/execution/test_executor.py::test_cancel_query_finished_while_preparing`. Both fail on ab5df9b3 and pass on 386b5e0b. Also checked, no findings: - Celery finalization: the guarded conditional UPDATE in `_commit_query_unless_stopped` against the Impala claim UPDATE serialises on the row lock in both orders. - `no_autoflush` around error recovery, and the `preserve_pending` refresh, which discards pending success metadata once a stop is observed. - `_store_results_in_backend` after a stop: the orphaned results key only expires via TTL. - Other engines: Trino's `prepare_cancel_query` is unchanged. Engines without a prepare hook never refresh status, so the new re-check is a no-op for them. - Legacy `sql_lab.execute_sql_statements` already preserves STOPPED. Tests run: `tests/unit_tests/dao/`, `tests/unit_tests/sql/execution/`, `test_impala.py`, `test_impala_pending.py`, `test_sqllab_error_stacktrace.py`, `sql_lab_test.py`. 605 passed. The only 8 failures are `generate_series` cases that need `duckdb`, which is not installed locally; they fail the same way without this change. pre-commit is clean on the changed files. Trino unit tests were not run locally (`trino` driver not installed). -- 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]
