aminghadersohi opened a new pull request, #45845: URL: https://github.com/apache/superset/pull/45845
### SUMMARY An Impala query that hits a SQL Lab time limit could keep running instead of ending as a failed, timed-out run. It was seen while testing https://github.com/apache/superset/pull/45232 against a real Impala 4.5.2 with a single admission slot, and the same behaviour exists without that change. SQL Lab raises its time limits into whatever the worker is executing: the synchronous `SQLLAB_TIMEOUT` alarm raises `SupersetTimeoutException`, and the Celery soft limit raises `SoftTimeLimitExceeded`. Each fires once. `ImpalaEngineSpec.handle_cursor` only handled the soft limit around its polling `sleep`/`status` calls, so: - A synchronous timeout raised while polling fell into the broad `except Exception: return` and was swallowed. The Run then waited for the whole query and failed from `fetch_data`'s own deadline instead of at `SQLLAB_TIMEOUT`. - Either limit raised while the progress log was fetched was swallowed as a failed `GetLog()` call. The limit had already fired, so nothing stopped the query until the Celery hard limit, and the row stayed `running` until then. - Either limit raised while `fetch_data` waited on the operation propagated without cancelling it, leaving it running on the coordinator and holding its admission slot. Changes: - `TIME_LIMIT_EXCEPTIONS` (the two exceptions above) is defined once in `db_engine_specs/base.py`. - `ImpalaEngineSpec.handle_cursor` cancels the operation and re-raises on either limit, and no longer swallows them in the `get_log()` call. `_cancel_operation` lets them through its cleanup RPCs. - `ImpalaEngineSpec.fetch_data` cancels the operation before re-raising either limit. - `HiveEngineSpec.handle_cursor` had the same swallow around `fetch_logs()` and now lets the limits through. Checked and unchanged: Presto and Trino do not wrap their polling in a broad `except`, and the base and Ocient `handle_cursor` have no polling loop. Not changed here: `get_sql_results` still hardcodes `soft_time_limit=21600` / `time_limit=21660` in the `@celery_app.task` decorator, so `SQLLAB_ASYNC_TIME_LIMIT_SEC` only affects the error message. Applying it to the task limits changes behaviour for every engine and needs a hook at app initialization, so it is left for a separate change. Operators can already set `task_annotations` for `sql_lab.get_sql_results` in `CELERY_CONFIG`. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A ### TESTING INSTRUCTIONS Unit tests with a mocked Impala/Hive cursor (a real Impala was not available): ``` 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_hive.py ``` The new tests fail on `master` (the sync-timeout and `get_log` cases fail on their assertions; the earlier hang shows up as an endless polling loop when the status mock is unbounded) and pass with this change. `test_sync_timeout_fails_the_run_and_cancels_the_operation` drives `execute_query` and checks that the Run fails with the timeout message and the operation is cancelled. Manual check against Impala: run a long query in SQL Lab with a small `SQLLAB_TIMEOUT` (sync) or a Celery soft time limit (async) and confirm the query fails at the limit and its operation is no longer running in the Impala web UI. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
