Repository navigation
fix(impala): end SQL Lab runs that hit a time limit instead of hanging - #45845
aminghadersohi wants to merge 14 commits into
Conversation
A SQL Lab time limit is raised once into whatever the worker is executing: the synchronous SQLLAB_TIMEOUT alarm raises SupersetTimeoutException and the Celery soft limit raises SoftTimeLimitExceeded. Impala's handle_cursor only treated the soft limit specially, so: - a synchronous timeout raised while polling fell into the broad `except Exception` and was swallowed, and the Run kept waiting for the whole query before failing from fetch_data's own deadline; - either limit raised while the progress log was fetched was swallowed as a failed GetLog() call, and with the limit already spent nothing stopped the query until the hard limit; - a limit raised while fetch_data waited on the operation left it running on the coordinator. Both limits are now cancelled and propagated from handle_cursor, including from the progress log call and the cancel helper, and fetch_data cancels the operation before propagating them. Hive's handle_cursor had the same swallow around fetch_logs and now lets the limits through. Presto and Trino do not wrap their polling in a broad except.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #45845 +/- ##
===========================================
+ Coverage 65.38% 84.70% +19.32%
===========================================
Files 3069 3070 +1
Lines 205465 206055 +590
Branches 48310 48394 +84
===========================================
+ Hits 134336 174535 +40199
+ Misses 68043 28215 -39828
- Partials 3086 3305 +219
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Product acceptance at Works as described for the phases the PR covers (a limit that lands while the operation is running or admission-queued):
Gap in Unchanged and pre-existing, as the description says: a threaded sync request (gthread) still has no time limit (no response at 45 s, here and at the merge-base). Not covered: the Hive change was not run against a live Hive server. Testing note: Impala's |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Request changes: the fix is right and well tested (16 of 17 new tests fail on master; on real Impala 4.5.2 a sync timeout now ends the run at the limit instead of about 20 s later), but the new cancel in the row-fetch time-limit handler can hang the worker. Details and a live-validated fix are inline.
…imit impyla's reconnect() only closes and reopens the socket or HTTP connection. TBufferedTransport and TSaslClientTransport keep their read and write buffers across close and open, so the unread rest of an interrupted status, log or row reply was parsed as the CancelOperation reply: on the binary and SASL transports the cancel blocked until the socket timeout and impyla resent it, and over HTTP it failed outright. Reset the transport instead: check every layer of the stack, close it, replace each layer's buffers with empty ones and reopen it. If the stack is not recognised or the reset fails, no cancel RPC is sent on it. Tests drive impyla over real binary, SASL PLAIN and HTTP transports against a local HiveServer2 stand-in, with the time limit raised right after a reply header is read.
When the transport reset is skipped or fails, the cancel RPC was not sent, but impyla's cursor close() (reached through closing() in the executor) and its finalizer still sent CloseOperation and CloseSession, reopening the transport with the interrupted reply still buffered. Mark the cursor closed on that path, and on a second time limit during the reset, so neither sends anything.
SUMMARY
At bb609de, signal-interrupted HiveServer2 replies cause the real cursor and connection to be abandoned before driver cleanup runs, rather than triggering another cancellation RPC.
tests/unit_tests/db_engine_specs/test_impala_transport.pyandtests/unit_tests/db_engine_specs/test_hive_transport.py.test_driver_close_timeout_prevents_transport_reopenexercises the CloseOperation-to-CloseSession exception path.test_sqlalchemy_ping_abandons_before_dialect_cursor_cleanupexercises both Impala and PyHive without calling spec.execute().test_row_limited_fetch_sends_nothing_after_a_skipped_cancelexercises the real cursor behind the wrapper.An explicit Stop and the local Impala polling deadline still request cancellation on an uninterrupted transport. After a signal-interrupted reply, no further RPC is attempted; an abandoned operation relies on server-side query/session timeouts. Cleanup failures are logged without replacing the original limit.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend exception and transport lifecycle changes.
TESTING INSTRUCTIONS
Run narrow files with
PYTHONPATH=.:superset-core/src pytest -q <file> -m 'not manual and not llm_compliance':tests/unit_tests/db_engine_specs/test_impala.pytests/unit_tests/db_engine_specs/test_impala_pending.pytests/unit_tests/db_engine_specs/test_impala_transport.pytests/unit_tests/db_engine_specs/test_hive.pytests/unit_tests/db_engine_specs/test_hive_transport.pytests/unit_tests/db_engine_specs/test_base.pytests/unit_tests/db_engine_specs/test_presto.pytests/unit_tests/sql/execution/test_*.pyfile separately.tests/unit_tests/sql_lab_test.pytests/unit_tests/models/core_test.py -k 'prequeries or get_raw_connection'The transport fixtures run local HiveServer2 stand-ins with real Thrift buffered, SASL PLAIN and HTTP transports. They interrupt replies after the header so unread bytes remain, then assert exception identity and the absence of subsequent RPCs. The starting-head verifier probe produced 16 failures; the same 16 cases pass with the shared guard. Full before/after file counts and sibling audit are recorded in the review-thread reply.
Pre-commit, including mypy, passed on this branch's changed files at bb609de.
Remaining: real Impala was not run, so server-side operation/admission-slot release and LDAP/GSSAPI negotiation are not proven. Threaded synchronous execution still has no SIGALRM enforcement, and the existing hardcoded Celery task limits are unchanged. No real-model checks were run.
ADDITIONAL INFORMATION