rebenitez1802 commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4448356573
##########
superset/db_engine_specs/impala.py:
##########
@@ -130,56 +143,289 @@ def execute(
) -> None:
try:
cursor.execute_async(query)
+ except TIME_LIMIT_EXCEPTIONS:
+ raise
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex) from ex
@classmethod
+ @guard_cursor
def fetch_data(cls, cursor: Any, limit: int | None = None) ->
list[tuple[Any, ...]]:
"""Wait for asynchronous operations using the public cursor API."""
- if callable(getattr(cursor, "execute_async", None)):
- from impala.error import Error
+ try:
+ if callable(getattr(cursor, "execute_async", None)):
+ from impala.error import Error
+
+ deadline = time.monotonic() + app.config["SQLLAB_TIMEOUT"]
+ try:
+ while cursor.is_executing():
+ if time.monotonic() >= deadline:
+ cls._cancel_operation(cursor)
+ raise TimeoutError(
+ "Timed out waiting for the Impala operation"
+ )
+ time.sleep(0.1)
+ if cursor.execution_failed():
+ # Fetching surfaces the driver's detailed asynchronous
error,
+ # including errors on statements that produce no
result set.
+ cursor.fetchall()
+ except Error as ex:
+ raise cls.get_dbapi_mapped_exception(ex) from ex
+ return super().fetch_data(cursor, limit)
+ except TIME_LIMIT_EXCEPTIONS:
+ cls._cancel_operation_on_time_limit(cursor)
+ raise
Review Comment:
🟡 Medium (non-blocking): this cancel now runs inside the short time-limit
gap of `get_df` callers such as alerts
This handler sends cancel, close-operation and close-session before the
limit propagates. On master, `fetch_data` sent nothing here, and the query
leaked instead. SQL Lab has 60 s between the Celery soft and hard limits.
Alerts and reports have only `ALERT_REPORTS_WORKING_TIME_OUT_LAG -
ALERT_REPORTS_WORKING_SOFT_TIME_OUT_LAG` = 9 s by default.
On Impala 4.5.2 a cancel can block for up to `FETCH_ROWS_TIMEOUT_MS` (about
10 s) while a fetch is in flight. With a fake server whose
`CloseImpalaOperation` takes 12 s, `fetch_data` returned after 13.0 s here and
1.0 s on master. Past 9 s the alert task is hard-killed before it records
ERROR, and its execution log stays "working".
There's no small fix that keeps the cancel, so this is your call. Options:
- **Cancel only, then abandon.** In this handler only, send just
`cancel_operation(reset_state=False)` and then abandon the cursor locally,
keeping the full close in `handle_cursor`. This shortens the cleanup but
doesn't bound the cancel itself.
- **Document the trade-off.** Recommend a driver network timeout, or a
larger alert lag, for deployments that run alerts on Impala.
##########
tests/unit_tests/db_engine_specs/test_impala_transport.py:
##########
@@ -0,0 +1,916 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Time-limit cancellation over real impyla Thrift transports.
+
+A local HiveServer2 stand-in serves impyla's binary, SASL PLAIN and HTTP
+transports. The time limit is raised right after a reply header is read, as a
+signal landing at that point would, so the rest of the reply is left in the
+client's buffers.
+"""
+
+from __future__ import annotations
+
+import signal
+import socket
+import struct
+import threading
+from collections.abc import Iterator
+from contextlib import closing
+from dataclasses import dataclass, field
+from typing import Any
+from unittest.mock import Mock, patch
+
+import pytest
+from celery.exceptions import SoftTimeLimitExceeded
+
+from superset.constants import QUERY_EARLY_CANCEL_KEY
+from superset.db_engine_specs.impala import ImpalaEngineSpec as spec # noqa:
N813
+from superset.errors import ErrorLevel, SupersetErrorType
+from superset.exceptions import SupersetTimeoutException
+from superset.sql.execution.executor import _LimitedCursor
+
+hs2 = pytest.importorskip("impala.hiveserver2")
+ttypes = pytest.importorskip("impala._thrift_gen.TCLIService.ttypes")
+TCLIService = pytest.importorskip("impala._thrift_gen.TCLIService.TCLIService")
Review Comment:
🟡 Medium (non-blocking): CI never runs these transport tests
The unit-test job installs `requirements/development.txt`, which has
`pyhive==0.7.0` but not impyla, thrift or thrift_sasl. So these `importorskip`
calls, and the matching ones in `test_hive_transport.py`, skip both files in
CI. That's about 1,200 lines, including the 30 new clean-transport regression
cases. They only run locally: with the drivers installed, both files pass (466
tests together with the mock suites).
Fix: add `hive` and `impala` to the extras list in
`requirements/development.in` (`-e .[development,bigquery,...]`), then
recompile `requirements/development.txt`. Or install those extras in the
unit-test workflow. Note that the `hive` extra pins `thrift>=0.25.0`, so the
suites should be re-checked on that version.
--
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]
