aminghadersohi commented on PR #45232:
URL: https://github.com/apache/superset/pull/45232#issuecomment-6145346748
Re-review of the full diff against master at `04cb0dad0f`, covering the
Impala spec, the Celery finalization and error-recovery stop races, Impala/Hive
cursor behaviour, and test coverage.
**Findings**
- **MEDIUM (fixed in `ab5df9b3bc`):**
`ImpalaEngineSpec.prepare_cancel_query` published `STOPPED` unconditionally.
Cancelling an async Impala query after it had finished (`SQLExecutor` handle
`cancel()`, or a SQL Lab stop that lost the race with the worker's terminal
commit) relabelled a `SUCCESS`, `FAILED` or `TIMED_OUT` query as `STOPPED`.
`get_result()` then stopped returning the results it had already stored. The
stop is now claimed with a conditional `UPDATE ... WHERE status NOT IN
(success, failed, timed_out)`. That write holds the row lock until commit, so
it serialises with the worker's guarded terminal commit
(`_commit_query_unless_stopped`) in either order. When the query has already
finished, the stop flag is not set and the stored status is left alone.
- Regression test: `test_prepare_cancel_query_keeps_finished_status`,
which uses a real SQLite session and a stale `RUNNING` read on the caller side.
Its finished-status cases fail without the fix.
No other valid findings:
- The 04cb0dad fix for error-recovery autoflush works. The task runs under
`no_autoflush`, and the only paths that write terminal metadata are the guarded
commit and `_stopped_query_payload`.
- Under REPEATABLE READ, the stop checks that preserve pending metadata can
read a stale snapshot. The guarded conditional `UPDATE` does a current read, so
correctness never depends on those checks.
- When a stop arrives while results are being written, a results-backend
entry can be left without a reference. It expires with the cache timeout, so it
is harmless.
**Tests**
- Ran the Impala and Hive engine-spec tests,
`tests/unit_tests/sql/execution/`, SQL Lab, DAO, queries and the stacktrace
tests: 588 passed. The impyla-backed stop tests were included.
- The only failures were 8 `generate_series` executor cases, which need
`duckdb`. That package isn't installed in this environment, and those cases
fail the same way without this change.
- pre-commit passes on the changed files, checked against the pinned ruff
0.9.7.
--
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]
