aminghadersohi commented on PR #45283: URL: https://github.com/apache/superset/pull/45283#issuecomment-6191254318
Commit: 6ebf453d10519565193ea61e769768ffe53d553b I chose to keep a durable STOPPED outcome and report Stop as accepted, with best-effort engine cancellation. Returning 422/raising or returning `False` after committing STOPPED contradicted the recorded outcome. An unavailable engine, rejected cancellation, or missing cancellation ID no longer produces that mismatch: the Stop API returns 200 and `handle.cancel()` returns `True`, matching the UI's success path. A statement that cannot be interrupted may finish, with its results discarded; committed database changes cannot be undone. This behavior is documented in the SQL Lab guide and UPDATING.md. A query that wins completion still returns 422/`False`. Regression controls, with the production hunks reverted and then restored: - `test_accepted_stop_reports_success_when_engine_cancel_fails`: 8 failures before → 8 passes after (pending/running, API/handle, unsupported/unreachable). - `test_accepted_stop_with_real_postgres_workers`: 7 failures and 5 passing controls before → 12 passes after. This runs the legacy worker and the actual Celery task, including `_execute_sql_statements`, against Postgres 17. Normal cancellation uses the real `PostgresEngineSpec.cancel_query`; failure cases verify the engine statement remains running after the accepted stop and its results are not published. Validation: - `pytest -q tests/unit_tests/sql_lab_test.py tests/unit_tests/sql/ tests/unit_tests/daos/ -m 'not manual and not llm_compliance'`: 2696 passed, 2 skipped. - `pytest -q tests/testcontainers/db_engine_specs/test_postgres.py`: 34 passed. - `pytest -q tests/integration_tests/queries/api_tests.py -k test_stop_query`: 3 passed. - `npm run test -- src/SqlLab/actions/sqlLab.test.ts -t postStopQuery`: 7 passed. - Staged-file `pre-commit run` and `pre-commit run mypy`: passed. An additional run against every branch-changed file passed its other applicable hooks but frontend type-checking reported TS6305 for unbuilt package declarations. Siblings checked: - `superset/daos/query.py` — fixed in 6ebf453d10519565193ea61e769768ffe53d553b - `superset/sql/execution/executor.py` — fixed in 6ebf453d10519565193ea61e769768ffe53d553b - `superset/sql_lab.py` — not affected: the lower-level cancellation boolean describes engine cancellation; its stale dispatch-race comment was updated. - `superset/sql/execution/celery_task.py` — not affected: the real-worker regression verifies STOPPED and no published results. - `superset/sql/execution/utils.py` — not affected: terminal-outcome guards remain covered by the passing unit suite. - `superset/db_engine_specs/trino.py` — not affected: guarded early-cancel tests pass. - `superset/db_engine_specs/impala.py` — not affected: guarded early-cancel tests pass. - `superset/queries/api.py` — not affected: accepted stops return 200; completed-query 422 tests pass. - `superset-frontend/src/SqlLab/actions/sqlLab.ts` — not affected: the passing stop-action tests cover the 200 success path and completed-query refresh. -- 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]
