aminghadersohi commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4444268898
##########
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:
Fixed in f73c0a47fcf61c8fadf58cfaf5be48767c14f5aa. I implemented the
proposed guard-state distinction: only interrupted RPCs abandon their
transport; clean polling/fetch limits cancel and release the Impala operation,
and clean Hive limits retain normal session cleanup. Non-reply cancellation
errors explicitly abandon the guard. The soft-limit cancellation test and the
clean-transport mock expectations are restored; the documentation distinguishes
the two cases.
Regression tests added by this PR:
- `test_polling_limit_cancels_on_clean_transport`: 24 cases covering
pending/running, polling/fetch entry points, soft/synchronous limits, and
binary/SASL/HTTP impyla transports.
- `test_limit_between_rpcs_preserves_session_cleanup`: 6 cases verifying
Hive still sends CloseOperation and CloseSession over binary/SASL/HTTP
transports.
Before: with the four engine/helper source files restored from
8c8604ec5640098c383131e810d4caf7bc5a96c4 and the added tests retained, the
regression selection reported `30 failed`; Impala omitted CancelOperation and
Hive omitted CloseSession. After: all 30 cases pass as part of the scoped
CI-command run, which reported `1031 passed` (exit 0), including the existing
interrupted-reply suites in `test_impala_transport.py` and
`test_hive_transport.py`.
Command (CI flags, two workers, scoped files):
```bash
SUPERSET_TESTENV=true PYTHONPATH=.:superset-core/src
PYTEST_XDIST_AUTO_NUM_WORKERS=2 python -m pytest -n auto --dist loadfile
--durations=0 --durations-min=0.5 --cov-report= --cov=superset
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_impala_transport.py
tests/unit_tests/db_engine_specs/test_hive.py
tests/unit_tests/db_engine_specs/test_hive_transport.py
tests/unit_tests/db_engine_specs/test_base.py
tests/unit_tests/db_engine_specs/test_presto.py
tests/unit_tests/sql/execution/test_executor.py --cache-clear --maxfail=50
--junit-xml=/tmp/45845-r7-final.xml -m 'not manual and not llm_compliance'
```
The negative control used the same pytest flags on the two transport files
with `-k 'test_polling_limit_cancels_on_clean_transport or
test_limit_between_rpcs_preserves_session_cleanup'`. Staged-file `pre-commit
run`, branch-file `pre-commit run --files ...`, and `pre-commit run mypy` each
exited 0. These tests use a local HiveServer2 stand-in with real driver
transports, not a live Impala server; live server-side admission-slot release
was not measured in this run.
Sibling search: `rg -n
'guard\.abandon|TIME_LIMIT_EXCEPTIONS|\.abandon\(|on_time_limit'
superset/db_engine_specs superset/utils/hive.py superset/sql/execution`
Siblings checked:
- superset/db_engine_specs/impala.py:146 — not affected: execution
re-raises; the generated RPC guard handles interrupted ExecuteStatement
replies, while clean cleanup remains enabled.
- superset/db_engine_specs/impala.py:175 — fixed in f73c0a47fc
- superset/db_engine_specs/impala.py:227 — fixed in f73c0a47fc
- superset/db_engine_specs/impala.py:284 — not affected: transport-reset
exception propagation performs local transport cleanup, not query polling or
unconditional client abandonment.
- superset/db_engine_specs/impala.py:391 — not affected: a limit during a
cleanup call abandons the cursor and propagates, preventing a second cleanup
attempt.
- superset/db_engine_specs/impala.py:401 — fixed in f73c0a47fc
- superset/db_engine_specs/impala.py:407 — fixed in f73c0a47fc
- superset/db_engine_specs/impala.py:423 — fixed in f73c0a47fc
- superset/db_engine_specs/impala.py:468 — fixed in f73c0a47fc
- superset/db_engine_specs/impala.py:485 — fixed in f73c0a47fc
- superset/db_engine_specs/hive.py:246 — fixed in f73c0a47fc
- superset/db_engine_specs/hive.py:527 — fixed in f73c0a47fc
- superset/utils/hive.py:60 — fixed in f73c0a47fc
- superset/utils/hive.py:90 — not affected: this handler surrounds the
generated RPC itself; interrupted replies still abandon before driver exception
handlers can send cleanup RPCs.
- superset/utils/hive.py:91 — not affected: abandonment here is restricted
to the interrupted generated RPC, not every cursor-context time limit.
- superset/db_engine_specs/base.py:2103 — not affected: delegates to the
engine callback; Impala and Hive callbacks distinguish clean transports,
including nested executor scopes.
- superset/sql/execution/executor.py:244 — not affected: truncation probes
delegate to engine fetch_data and re-raise limits without abandoning the client
themselves.
- superset/sql/execution/executor.py:927 — not affected: the outer cursor
context uses the same engine callback and does not independently abandon a
clean transport.
- superset/db_engine_specs/base.py:148 — not affected: exception-type
declaration only.
- superset/db_engine_specs/impala.py:37 — not affected: exception-type
import only.
- superset/db_engine_specs/hive.py:47 — not affected: exception-type import
only.
- superset/utils/hive.py:31 — not affected: exception-type import only.
- superset/sql/execution/executor.py:75 — not affected: exception-type
import only.
I have left the thread unresolved for independent verification and have not
requested a reviewer.
--
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]
