aminghadersohi commented on PR #45232: URL: https://github.com/apache/superset/pull/45232#issuecomment-6201791722
In response to https://github.com/apache/superset/pull/45232#pullrequestreview-5537157969 : **Async-cancel scope:** The scope expansion is fixed in daf2d551660983edb8a8f38f150fdb3619dbc4a4. I reverted the executor-owned STOPPED write, the Celery error-handler stop recheck, the developer-doc section and their persistence tests in daf2d551660983edb8a8f38f150fdb3619dbc4a4. The error handler is byte-identical to master. I retained `test_stop_seen_during_execution_skips_mutation_commit` (UPDATE/CTAS, both stop signals). Regression: `test_executor_cancel_does_not_commit_caller_session` exercises PostgreSQL, MySQL, Hive and Presto through the real async handle and separate SQLite metadata sessions, including unrelated pending caller work. Together with the strengthened `test_cancel_query_early_cancel_flag`, all five cases fail against 758e87f60fa44f24e8d7297aa61a4cfb3d4bf28c and pass here. The affected suites pass 2,753 tests, with the Impala driver installed; pre-commit passes on branch-changed files. Siblings checked: - superset/sql/execution/executor.py:1275 — fixed in daf2d551660983edb8a8f38f150fdb3619dbc4a4 - superset/sql/execution/executor.py:1430 — fixed in daf2d551660983edb8a8f38f150fdb3619dbc4a4 - superset/sql/execution/executor.py:1469 — fixed in daf2d551660983edb8a8f38f150fdb3619dbc4a4 - superset/sql/execution/celery_task.py:76 — fixed in daf2d551660983edb8a8f38f150fdb3619dbc4a4 - superset/sql/execution/celery_task.py:371 — fixed in daf2d551660983edb8a8f38f150fdb3619dbc4a4 - superset/sql_lab.py:873 — not affected: legacy cancellation delegates metadata ownership to QueryDAO, not the async handle. - superset/daos/query.py:79 — not affected: its existing stop claim and terminal-outcome tests remain unchanged. - superset/db_engine_specs/impala.py:244 — not affected: its engine-specific claim already commits STOPPED; the live-cursor tests still pass. - superset/db_engine_specs/trino.py:527 — not affected: its pre-existing prepare hook owns the early-flag commit; the generic executor adds no commit. - superset/db_engine_specs/hive.py:665 — not affected: implicit-cancel capability is unchanged; the Hive regression case pins caller transaction ownership. - superset/db_engine_specs/presto.py:1477 — not affected: implicit-cancel capability is unchanged; the Presto regression case pins caller transaction ownership. - superset/mcp_service/worker.py:794 — not affected: MCP uses its own cancellation path, not SQLExecutor cancellation. **Merge coupling:** the common stop-before-engine-cancel design belongs in https://github.com/apache/superset/pull/45283 . Merge order: that PR first, then this PR after merging `origin/master` into this branch; I am not duplicating that redesign here. Remaining: this branch is not yet stacked on the shared design. `git merge-tree --write-tree HEAD origin/pr-45283` exits 1 with conflicts in `UPDATING.md`, `daos/query.py`, `impala.py`, `celery_task.py` and `test_celery_task.py`. General async/error/post-commit cancellation behavior belongs in that PR. Whether its HTTP-only Impala path avoids both reported failures without the polling trade-off still needs real-Impala acceptance; I cannot claim that result from unit tests. -- 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]
