aminghadersohi commented on code in PR #44737:
URL: https://github.com/apache/superset/pull/44737#discussion_r4129381991
##########
superset/db_engine_specs/impala.py:
##########
@@ -147,33 +149,29 @@ def handle_cursor(cls, cursor: Any, query: Query) -> None:
# the query was still executed
# modified in stop_query in views / core.py is reflected here.
# stop query
- if query.extra.get(QUERY_EARLY_CANCEL_KEY):
+ if query.extra.get(QUERY_EARLY_CANCEL_KEY) or query.status in (
+ QueryStatus.STOPPED,
+ QueryStatus.TIMED_OUT,
+ ):
Review Comment:
The read that gates the cancel is the same one master already used for the
early-cancel flag on the line above, so the snapshot exposure is unchanged by
this PR. It is also the established polling pattern here: hive.py:429 does the
identical db.session.refresh + re-query with query.status == STOPPED, and
presto.py:1429 re-queries and checks STOPPED/TIMED_OUT. Leaving it consistent
with those rather than diverging in one spec.
##########
superset/db_engine_specs/impala.py:
##########
@@ -147,33 +149,29 @@ def handle_cursor(cls, cursor: Any, query: Query) -> None:
# the query was still executed
# modified in stop_query in views / core.py is reflected here.
# stop query
- if query.extra.get(QUERY_EARLY_CANCEL_KEY):
+ if query.extra.get(QUERY_EARLY_CANCEL_KEY) or query.status in (
+ QueryStatus.STOPPED,
+ QueryStatus.TIMED_OUT,
+ ):
cursor.cancel_operation()
cursor.close_operation()
cursor.close()
break
- # updates progress info by log
- try:
- log = cursor.get_log() or ""
- except Exception: # pylint: disable=broad-except
- logger.warning("Call to GetLog() failed")
- log = ""
+ # Pending/initialized operations have no execution progress
yet.
+ if status == "RUNNING_STATE":
+ try:
+ log = cursor.get_log() or ""
+ except Exception: # pylint: disable=broad-except
+ logger.warning("Call to GetLog() failed")
+ log = ""
- if log:
- match = QUERY_PROGRESS_REGEX.match(log)
- if match:
+ if match := QUERY_PROGRESS_REGEX.match(log):
progress = int(match.groupdict()["query_progress"])
- logger.debug(
- "Query %s: Progress total: %s", str(query_id),
str(progress)
- )
- needs_commit = False
- if progress > query.progress:
- query.progress = progress
- needs_commit = True
-
- if needs_commit:
- db.session.commit() # pylint:
disable=consider-using-transaction
+ logger.debug("Query %s: Progress total: %s", query_id,
progress)
+ if progress > query.progress:
+ query.progress = progress
+ db.session.commit() # pylint:
disable=consider-using-transaction
Review Comment:
Query.progress is Column(Integer, default=0) in models/sql_lab.py:164, and
create_query() in sqllab_execution_context.py never passes progress, so the ORM
default writes 0 on every insert and the column is not NULL for a query that
reaches handle_cursor. The comparison is also unchanged from master and
hive.py:471 does the same thing.
##########
superset/db_engine_specs/impala.py:
##########
@@ -147,33 +149,29 @@ def handle_cursor(cls, cursor: Any, query: Query) -> None:
# the query was still executed
# modified in stop_query in views / core.py is reflected here.
# stop query
- if query.extra.get(QUERY_EARLY_CANCEL_KEY):
+ if query.extra.get(QUERY_EARLY_CANCEL_KEY) or query.status in (
+ QueryStatus.STOPPED,
+ QueryStatus.TIMED_OUT,
+ ):
cursor.cancel_operation()
cursor.close_operation()
cursor.close()
Review Comment:
Real, and the outer catch-all hid it. cancel_operation() is now wrapped and
the close_operation()/close() calls run regardless, so a failed cancel RPC no
longer leaves the operation open on the coordinator. Added
test_failed_cancel_still_releases_the_operation, which fails without the
change. Fixed in 2458ced432.
--
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]
