aminghadersohi commented on PR #45283:
URL: https://github.com/apache/superset/pull/45283#issuecomment-6193000030
Commit: 38d0d3ff5679c48e861136370981d9dc3a36b9f8
- Both SQL Lab worker paths serialize the final Stop check and target commit
using the metadata query-row lock, then record the target-committed marker
before releasing it. A Stop that loses this boundary returns HTTP 422
(`cancel()` returns `False`), including while results are still being prepared.
A winning Stop prevents the worker's final transactional commit.
- A Celery soft time limit after an accepted Stop preserves the stopped
outcome and its end time.
- Repeated Stop requests preserve the original end time while retaining
best-effort cancellation retries. Cancellation-metadata merge/commit exceptions
after durable acceptance no longer turn that acceptance into an error response.
Both API and async-handle paths use the shared finalization helper.
- The synchronous inline payload-size check runs before publishing SUCCESS,
so Stop cannot observe a SUCCESS that the size check subsequently changes to
FAILED.
- The docs distinguish accepted Stop from best-effort engine cancellation,
remove the contradictory failure-toast description (including documenting the
failure toast for a rejected commit-boundary Stop that still has running
state), and describe autocommit/implicit/explicit SQL commits, SQLite's
check-to-commit window, and the cross-database metadata-commit failure
limitation. The Stop response description was regenerated with `superset
update-api-docs` (one OpenAPI description changed).
- Removed unused `is_autocommit`, `autocommit_by_default`, and `on_commit`
code and their dedicated tests.
Regression tests (baseline `586cbd6cf2d2be9f4f32325d38c130513467836c` and
production-revert run → restored production):
| Test | Before / reverted | After |
| --- | --- | --- |
| `test_stop_at_target_commit_is_serialized` | 24 failed | 24 passed |
| `test_soft_timeout_after_accepted_postgres_stop` | 4 failed, 4 passed
(legacy controls) | 8 passed |
| `test_stop_acceptance_survives_post_commit_failure` | 6 failed | 6 passed |
| `test_sync_payload_limit_stop_response_matches_recorded_outcome` | 1
failed | 1 passed |
The PostgreSQL cases exercise both production workers, API/handle callers,
INSERT/CTAS, and successful/unreachable/missing-ID cancellation. The
commit-boundary cases also hold result publication pending while Stop checks
the committed marker. The existing execution-time Stop cases remain covered.
Trino/Impala early-cancel and SQLite paths were checked in the unit suites;
SQLite's remaining commit window is documented.
Validation:
- `pytest tests/unit_tests/sql_lab_test.py tests/unit_tests/sql/
tests/unit_tests/daos/ -m 'not manual and not llm_compliance'`: **2709 passed,
2 skipped**.
- `pytest tests/testcontainers/db_engine_specs/test_postgres.py -m
testcontainers`: **114 passed**.
- `pytest
tests/unit_tests/db_engine_specs/{test_base,test_sqlite,test_trino}.py`: **328
passed**.
- `pytest tests/integration_tests/queries/api_tests.py -k stop_query`: **3
passed**.
- `npm test -- src/SqlLab/actions/sqlLab.test.ts`: **86 passed, 1 skipped**.
- `pre-commit run`: passed on this commit's staged files; mypy passed on the
exact branch file list. All other exact-branch hooks passed with the frontend
type-check hook skipped after its failed run. The branch-wide frontend type
check reports missing generated declarations (`TS6305`); it did not pass.
Siblings checked:
- `superset/sql_lab.py:457` — fixed in
38d0d3ff5679c48e861136370981d9dc3a36b9f8 (legacy target commit and inline
payload-size publication).
- `superset/sql/execution/celery_task.py:87` — fixed in
38d0d3ff5679c48e861136370981d9dc3a36b9f8 (Celery timeout and target commit).
- `superset/daos/query.py:63` — fixed in
38d0d3ff5679c48e861136370981d9dc3a36b9f8 (API Stop acceptance and repeated
Stop).
- `superset/sql/execution/executor.py:1430` — fixed in
38d0d3ff5679c48e861136370981d9dc3a36b9f8 (async-handle Stop acceptance and
repeated Stop).
- `superset/sql/execution/utils.py:137` — fixed in
38d0d3ff5679c48e861136370981d9dc3a36b9f8 (shared timestamp, commit-boundary,
and post-acceptance handling).
- `superset/db_engine_specs/trino.py:504` — not affected: preparation stages
early-cancel intent without independently committing it; the guarded stop
helper merges that intent. The existing terminal-metadata/early-cancel tests
pass in the DAO suite.
- `superset/sql/execution/celery_task.py:536` — fixed in
38d0d3ff5679c48e861136370981d9dc3a36b9f8 (target commit).
- `superset/sql/execution/utils.py:196` — fixed in
38d0d3ff5679c48e861136370981d9dc3a36b9f8 (target-commit row lock and marker).
- `superset/sql/execution/utils.py:215` — fixed in
38d0d3ff5679c48e861136370981d9dc3a36b9f8 (post-acceptance handling).
- `superset/db_engine_specs/impala.py:234` — not affected: preparation only
stages early-cancel intent; the caller's guarded stop transaction persists it.
The existing DAO early-cancel tests pass.
Remaining: SQLite metadata has the documented final-check-to-target-commit
window. Metadata persistence can also fail after a target commit; the two
databases cannot commit atomically. Autocommit, implicit-commit DDL, and
explicit SQL commits during execution cannot be rolled back by Stop. The
frontend-wide type check remains blocked by missing generated declarations.
--
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]
