rebenitez1802 commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4443537755
##########
superset/db_engine_specs/impala.py:
##########
@@ -234,13 +454,9 @@ def handle_cursor(cls, cursor: Any, query: Query) -> None:
pending_sleep_interval *= 2
time.sleep(sleep_interval)
status = cursor.status()
- except SoftTimeLimitExceeded:
- # SQL Lab's own handler marks the query TIMED_OUT once this
propagates;
- # the operation is cancelled first so it does not keep running on
the
- # coordinator while fetch_data() waits on it.
- cls._cancel_operation(cursor, query_id)
+ except TIME_LIMIT_EXCEPTIONS:
raise
Review Comment:
🔴 High: time limits between RPCs abandon the connection instead of
cancelling the query
`cursor_context` (entered through `@guard_cursor`) calls `guard.abandon` for
every time limit, including one raised in `handle_cursor`'s poll `time.sleep`.
At that point no RPC is in flight and the transport is clean. This `except
TIME_LIMIT_EXCEPTIONS` then only re-raises. With a 5 s poll interval and
millisecond RPCs, that is where most polling-phase limits land.
On real Impala 4.5.2, a Celery soft limit while polling ended the run on
time. But in 3 of 3 runs the query kept running to completion and still held
its admission slot 120 s later, with `disconnected_session_timeout=900`. Master
cancels it immediately. A sync limit while polling and a limit inside GetLog
leak the same way. On Hive, `conn.close` becomes a local drop, so master's
`CloseSession` is never sent. `test_soft_time_limit_cancels_the_operation` was
inverted to assert `cancel_operation.assert_not_called()`, and its docstring
still says "cancels".
Fix: abandon only when the guard actually caught an interrupted RPC;
otherwise run master's cleanup.
- Add `ThriftClientGuard.interrupted` (`self._exception is not None`) and a
no-op `on_time_limit(ex)`, and return `guard.on_time_limit` from the Impala and
Hive `prepare_cursor` instead of `guard.abandon`.
- In this `except TIME_LIMIT_EXCEPTIONS`, call `_cancel_operation` unless
the cursor is already abandoned or closed, or the guard is interrupted. Do the
same in `fetch_data`.
- In `_cancel_operation`'s non-reply-error branch, call `guard.abandon(ex)`
explicitly.
With that change, the real-transport suites (`test_impala_transport.py`,
`test_hive_transport.py`) all pass. On real Impala, a limit while polling
cancels the query, and the mid-reply case still doesn't hang. The mock tests
that raise the limit from `time.sleep` or Mock cursor methods never go through
the guard, so they model the clean-transport case and need to expect a cancel.
--
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]
