aminghadersohi commented on PR #45232: URL: https://github.com/apache/superset/pull/45232#issuecomment-6173533153
@rebenitez1802 thanks for the thorough review. I split the PR and worked through each finding. Head is 70fec12f161c952b7fc52b7244fc62dc1063077f. **🟡 Description no longer matches the diff.** I split it. 8825befaa584ab83c18aad509eefdad83931057a moves the all-engine Celery finalization rework and the `sql_lab.py` `durable`/`on_commit` changes out of this PR; they are now https://github.com/apache/superset/pull/45283. This PR keeps the Impala routing, the finished-status checks it needs, and the one Celery change Impala executor stops depend on: a cancelled impyla cursor clears `cursor.description`, so the stop has to be read from status or the early-cancel flag (`test_cancelled_impala_cursor_is_not_finalized_as_success`). I rewrote the description to cover every behaviour change left in the diff, including that Celery change and its two extra metadata reads. **🟡 A stop no longer reaches Impala when the worker isn't polling.** Not fixed in code; the trade-off stands and needs your sign-off. Documented in 34f6466f98de680ee9a144d86f6c5df0467f1bf2 (the `prepare_cancel_query` docstring lists dead or restarted workers, a stop during `fetch_data`, and polling that ended because `cursor.status()` raised), in 8b8924e9aa (UPDATING.md note for operators who relied on the HTTP cancel), and in the description, which also names the removed `test_stop_with_cancel_id_uses_http` guarantee. **🟢 `stop_query` writes STOPPED again after Impala's claim.** Fixed in 34f6466f98de680ee9a144d86f6c5df0467f1bf2. `stop_query` writes nothing once the engine spec has committed the stop, and otherwise writes STOPPED with a conditional UPDATE that skips finished rows. Impala's claim records the end time itself, and the stale comment is updated. One correction to the finding: the status itself was not rewritten, because SQLAlchemy leaves an unchanged column out of the UPDATE, but `end_time` was overwritten. Regression tests, both real SQLite sessions with a second session as the worker, failing before and passing after: - `test_stop_query_does_not_rewrite_impala_claim`: the worker commits SUCCESS with an end time just before `stop_query` commits. Before the fix the end time was overwritten. - `test_stop_query_keeps_status_committed_after_its_check`: the same race for any engine. Before the fix the row ended `stopped` instead of `success`. **🟢 A declined claim leaves a `FOR UPDATE` lock open.** Fixed in 34f6466f98de680ee9a144d86f6c5df0467f1bf2: the declined branch commits after the locking read. Regression test: `test_declined_impala_claim_ends_transaction` (SQL Lab and executor paths). A second connection writes the row after the declined stop. Before the fix that write failed with `database is locked`. **🟢 `_handle_query_error` raises after committing.** This moved with the split, so it is fixed in https://github.com/apache/superset/pull/45283 (95efdd91ee): the status enum is computed before the commit and `expire_on_commit=False` is dropped from the tests. With production-style sessions the 14 cases you found failed with `AttributeError` before the fix and pass after. **🟢 `durable` relies on `conn.commit()`.** Moved to https://github.com/apache/superset/pull/45283. A comment there (d117709fde) notes that autocommit engines make DML and CTAS permanent at statement completion, so a stop before the pre-commit check still ends STOPPED. That PR's description also lists it as a known limitation. Not fixed. **🟢 Redundant metadata reads.** Moved to https://github.com/apache/superset/pull/45283 (d117709fde). It drops the post-cleanup and `_finalize` re-checks. A successful SELECT goes from 11 to 9 statements in my harness (master: 5): one full refresh, two status-only reads, the guarded UPDATE and the final write. `test_successful_select_reads_stop_state_once_per_checkpoint` fails before (3 full refreshes) and passes after (1). The two status reads stay because the guarded UPDATE only checks status, and an executor stop can publish only the early-cancel flag. In this PR the Celery path keeps the two reads described above. **🟢 Finished-status set duplicated; return values disagree.** Fixed in 34f6466f98de680ee9a144d86f6c5df0467f1bf2. `FINISHED_QUERY_STATUSES` lives in `superset/common/db_query_status.py` and is used by `impala.py`, `executor.py`, `sql_lab.py` and `daos/query.py`. `sql_lab.cancel_query` now returns False like `SQLExecutor._cancel_query` for a query that finished while the stop was prepared; `stop_query` checks the finished status before raising. Covered by `test_finished_query_stop_bypasses_repeatable_read_snapshot`, which asserts False on both paths. **🟢 Tests that build the fix into their mocks.** - `test_poll_sees_stop_committed_after_its_snapshot` is replaced by a real-session version in `tests/unit_tests/dao/query_stop_isolation_test.py`. It uses SQLite in WAL mode with explicit `BEGIN`, so the worker's transaction keeps its first snapshot. With the per-poll commit from ce10e4f068 reverted it fails (`cancel_operation` not called); at head it passes. - `test_finished_query_stop_bypasses_repeatable_read_snapshot` stays mocked. SQLite cannot model InnoDB's current read on a stale snapshot; it fails the write with `SQLITE_BUSY_SNAPSHOT`. The docstring now says so. Remaining: nothing in unit tests protects the locking read on MySQL or Postgres; only the real-Impala + MySQL acceptance run does. - `test_publish_query_result_preserves_failure` moved with the split and now uses a real session with the FAILED status committed first, as `_store_results_in_backend` does. **🟢 Real-Impala acceptance predates the last commits.** Not done yet. The Impala 4.5.2 + MySQL acceptance matrix has not been re-run at 70fec12f161c952b7fc52b7244fc62dc1063077f. Remaining: a re-run at head. **🟢 Red `check-openapi-spec-drift`.** Merged master in 1713bbfe06 (merge commit, no rebase). CI is running at the new head. Siblings checked: - superset/sql_lab.py:891 — fixed in 34f6466f98: uses `FINISHED_QUERY_STATUSES` and returns False. - superset/sql/execution/executor.py:1465 — fixed in 34f6466f98: uses `FINISHED_QUERY_STATUSES`. - superset/daos/query.py:73 — fixed in 34f6466f98: both finished checks use the shared constant and the STOPPED write is conditional. - superset/db_engine_specs/trino.py:374 — not affected: checks STOPPED or TIMED_OUT, a different set. - superset/commands/sql_lab/execute.py:137 — not affected: checks RUNNING, PENDING or TIMED_OUT, a different set. - superset/sql/execution/executor.py:545 — not affected: builds a TIMED_OUT error result, not a status set. Searched with `grep -rn "FINISHED_QUERY_STATUSES\|QueryStatus.TIMED_OUT,$\|refresh(.*with_for_update" superset`. The `with_for_update` refresh in https://github.com/apache/superset/pull/45283 (`_commit_query_unless_stopped`) is followed by `_stopped_query_payload`, which commits, so it does not hold the lock. Tests: `pytest tests/unit_tests/db_engine_specs/test_impala.py tests/unit_tests/db_engine_specs/test_impala_pending.py tests/unit_tests/db_engine_specs/test_cancel_query_validation.py tests/unit_tests/sql_lab_test.py tests/unit_tests/sql/execution tests/unit_tests/dao tests/unit_tests/test_sqllab_error_stacktrace.py tests/unit_tests/queries`: 677 passed. 8 `test_apply_limit_generate_series_returned_rows` cases fail locally because `duckdb` is not installed here; they are not touched by this PR. -- 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]
