aminghadersohi opened a new pull request, #45283: URL: https://github.com/apache/superset/pull/45283
### SUMMARY Split out of https://github.com/apache/superset/pull/45232 at review, so that PR stays scoped to the Impala stop routing. This PR hardens how a SQL Lab stop races a query that is finishing, for every engine. Changes on the Celery executor path (`superset/sql/execution/celery_task.py`, used by `SQLExecutor.execute_async`): - After execution, a stop is detected by STOPPED status or the early-cancel flag, not only by an empty result list. A cancelled cursor can clear `cursor.description`, which made a stopped query look like finished DML. A stopped mutation or CTAS is not committed on the target database. - A stop keeps the end time it recorded. - Terminal writes (SUCCESS, a results-backend FAILED, the error handler's FAILED or TIMED_OUT) go through a conditional `UPDATE ... WHERE status != 'stopped'`, so they cannot overwrite a stop committed by another request. The task runs under `no_autoflush`, so error recovery cannot flush stale metadata over a stop. - A stop that lands during result serialization or the results-backend write ends STOPPED, and its results key is not published. It is caught by status reads after the backend write and before the guarded publish; the results are still serialized and written in that case. - Once the target database has committed a mutation or CTAS, a later stop no longer masks it: the query ends SUCCESS, or FAILED if a later step (serialization, the results backend, closing the connection) fails. - `_handle_query_error` returned `query.status.value` after its commit. With the default `expire_on_commit`, `status` reloads as a plain `str`, so every error path committed FAILED or TIMED_OUT and then raised `AttributeError`. This bug is also on master. The enum is kept from before the commit. Change on the older SQL Lab path (`superset/sql_lab.py`, `get_sql_results`), all engines: a stop that races `conn.commit()` of a mutation or CTAS now ends SUCCESS (or FAILED if a later step fails) instead of STOPPED, because the rows or table are already written. Known limitation: engines that autocommit each statement (Impala, Hive, Trino) make DML and CTAS permanent as soon as the statement finishes, and `conn.commit()` does nothing there. A stop that lands between statement completion and the pre-commit check still ends STOPPED with the data already written. The window is small; it is noted in a code comment. Metadata cost: a successful SELECT through `execute_sql_task` issues 9 metadata statements in the unit-test harness, against 5 on master: one full refresh before the commit decision, two status-only reads, the guarded UPDATE and the final write. `test_successful_select_reads_stop_state_once_per_checkpoint` pins the number of re-reads. The first commit ("preserve executor stops before Celery finalization") is also in https://github.com/apache/superset/pull/45232, which needs it for Impala executor stops. Whichever merges second needs a small merge of `test_celery_task.py`. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A (backend only). ### TESTING INSTRUCTIONS ``` pytest tests/unit_tests/sql/execution tests/unit_tests/sql_lab_test.py tests/unit_tests/test_sqllab_error_stacktrace.py ``` Several tests use a file-backed SQLite metadata database with a second session as the competing request, with the default `expire_on_commit`, rather than mocked sessions: - `test_failure_after_stop_raced_commit_reports_failed_not_stopped` (Celery) and its `sql_lab_test.py` counterpart: committed work that fails afterwards ends FAILED, not STOPPED. - `test_task_error_lookup_does_not_flush_over_a_stop`: error recovery does not flush stale metadata over a committed stop. - `test_publish_query_result_preserves_failure`: a committed backend failure survives publication. - `test_successful_select_reads_stop_state_once_per_checkpoint`: re-read count on the success path. Manual: with the Celery executor, run a long `CREATE TABLE ... AS SELECT` or `INSERT` in SQL Lab and press Stop at different points. A stop before the statement finishes ends stopped with nothing committed; a stop after the commit ends success. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
