aminghadersohi commented on code in PR #45859:
URL: https://github.com/apache/superset/pull/45859#discussion_r4444047043
##########
superset/sql/execution/celery_task.py:
##########
@@ -661,6 +661,14 @@ def _execute_sql_statements(
level=ErrorLevel.ERROR,
)
) from ex
+ except Exception:
Review Comment:
Verified at 2a82a8a4a2: only `QueryStoppedException` is handled quietly now.
Restoring the bare `except Exception:` fails
`test_only_an_engine_stop_signal_is_reported_quietly[error2-True-stopped-True]`.
Only Impala raises it and `celery_task` is its only catcher.
##########
docs/admin_docs/configuration/configuring-superset.mdx:
##########
@@ -835,6 +835,16 @@ pending when result fetching begins, the additional
polling wait is bounded by
a timeout. Configure the driver's network timeout separately to bound
individual
RPC calls.
+Impala reports a streaming `SELECT` as finished as soon as its rows can be
fetched,
+so most of such a query runs while SQL Lab reads its results. While it
fetches, the
+worker checks for a stop between result RPCs, at most once per
+`DB_POLL_INTERVAL_SECONDS`, and cancels the query on its own connection. Each
result
+RPC returns within Impala's `FETCH_ROWS_TIMEOUT_MS` query option (10 seconds by
+default, available from Impala 3.4) even when no rows are ready, so a stop
takes
+effect within about `FETCH_ROWS_TIMEOUT_MS` plus `DB_POLL_INTERVAL_SECONDS`;
+lowering either makes stops take effect sooner. Impala checks for cancellation
between row batches, so a batch whose
Review Comment:
Verified at 2a82a8a4a2: the suggested text is in the docs verbatim and the
UPDATING.md note matches it.
##########
superset/db_engine_specs/impala.py:
##########
@@ -197,20 +306,9 @@ def handle_cursor(cls, cursor: Any, query: Query) -> None:
try:
status = cursor.status()
while status in unfinished_states:
- # End the read transaction first. Under REPEATABLE READ,
refreshing
- # inside one long transaction keeps returning the first
snapshot,
- # so a Stop committed by another request would never be seen.
- db.session.commit() # pylint:
disable=consider-using-transaction
- db.session.refresh(query)
- # Stop was requested: either before a cancel handle was
published
- # (early-cancel flag) or through SQL Lab's stop, which persists
- # STOPPED once cancel_query() succeeds.
- if (
- query.extra.get(QUERY_EARLY_CANCEL_KEY)
- or query.status == QueryStatus.STOPPED
- ):
+ if cls._stop_requested(query):
Review Comment:
Pre-existing: master's `handle_cursor` wraps the same polling loop in the
same `except Exception: return`, so a `SupersetTimeoutException` from
`cursor.status()` is swallowed on master and this branch alike. This PR doesn't
change that handler, and its new fetch-time check re-raises the exception.
--
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]
