I3eka commented on code in PR #45561:
URL: https://github.com/apache/superset/pull/45561#discussion_r4377042599
##########
superset/sql/execution/executor.py:
##########
@@ -680,16 +684,24 @@ def execute(
QueryStatus.TIMED_OUT,
"Query exceeded the timeout limit",
start_time,
+ query=query,
)
except SupersetSecurityException as ex:
- return self._create_error_result(QueryStatus.FAILED, str(ex),
start_time)
- except (OAuth2RedirectError, OAuth2Error):
+ return self._create_error_result(
+ QueryStatus.FAILED, str(ex), start_time, query=query
+ )
+ except (OAuth2RedirectError, OAuth2Error) as ex:
# Let OAuth2 exceptions propagate so callers (MCP, API) can
# handle them with context-appropriate responses.
+ self._create_error_result(
+ QueryStatus.FAILED, str(ex), start_time, query=query
+ )
raise
except Exception as ex:
error_msg = self.database.db_engine_spec.extract_error_message(ex)
- return self._create_error_result(QueryStatus.FAILED, error_msg,
start_time)
+ return self._create_error_result(
+ QueryStatus.FAILED, error_msg, start_time, query=query
+ )
Review Comment:
Fixed in 8698a1afe6. Cache writes now catch and log failures inside
`_store_in_cache()`, so a cache outage cannot replace the successful SQL result
or rewrite the committed Query as failed. The regression forces
`data_cache.set()` to raise and checks both the returned rows and the stored
success state through a fresh metadata session.
##########
superset/sql/execution/executor.py:
##########
@@ -1008,14 +1021,41 @@ def _create_error_result(
:param start_time: Start time for calculating execution duration
:param partial_results: Optional list of StatementResult from
successful
statements before the failure
+ :param query: Persisted audit record, if execution reached that stage
:returns: QueryResult with error status
"""
- from superset_core.queries.types import QueryResult as QueryResultType
+ from superset_core.queries.types import (
+ QueryResult as QueryResultType,
+ QueryStatus,
+ )
+
+ from superset.sql.execution.utils import commit_query_unless_stopped
+
+ query_id = None
+ if query is not None:
+ try:
+ # Read the persisted identity without loading expired
attributes.
+ query_id = inspect(query).identity[0]
+ # A failed metadata flush must be rolled back before recording
failure.
+ if not db.session.is_active:
+ db.session.rollback() # pylint:
disable=consider-using-transaction
+ query.status = status.value
+ query.error_message = error_message
+ query.end_time = now_as_float()
+ if not commit_query_unless_stopped(query):
+ status = QueryStatus.STOPPED
Review Comment:
Fixed in 8698a1afe6. The STOPPED branch now commits after reading the
winning status and error message, releasing the transaction opened by the
guarded refresh. The concurrent-Stop regression asserts `not
session.in_transaction()` before its later caller commit, then checks the
preserved status, end time, and cancellation metadata from another session. The
success path uses the same guard and closes its STOPPED transaction too.
##########
superset/sql/execution/executor.py:
##########
@@ -1008,14 +1021,41 @@ def _create_error_result(
:param start_time: Start time for calculating execution duration
:param partial_results: Optional list of StatementResult from
successful
statements before the failure
+ :param query: Persisted audit record, if execution reached that stage
:returns: QueryResult with error status
"""
- from superset_core.queries.types import QueryResult as QueryResultType
+ from superset_core.queries.types import (
+ QueryResult as QueryResultType,
+ QueryStatus,
+ )
+
+ from superset.sql.execution.utils import commit_query_unless_stopped
+
+ query_id = None
+ if query is not None:
+ try:
+ # Read the persisted identity without loading expired
attributes.
+ query_id = inspect(query).identity[0]
+ # A failed metadata flush must be rolled back before recording
failure.
+ if not db.session.is_active:
+ db.session.rollback() # pylint:
disable=consider-using-transaction
+ query.status = status.value
+ query.error_message = error_message
Review Comment:
Fixed in 8698a1afe6: failure finalization now uses `query.end_time or
now_as_float()`. Loading an expired timestamp is wrapped in `no_autoflush` so
it cannot flush staged failure metadata before the Stop guard. A regression
verifies that an existing end time is preserved.
##########
superset/sql/execution/executor.py:
##########
@@ -680,16 +683,24 @@ def execute(
QueryStatus.TIMED_OUT,
"Query exceeded the timeout limit",
start_time,
+ query=query,
)
except SupersetSecurityException as ex:
- return self._create_error_result(QueryStatus.FAILED, str(ex),
start_time)
- except (OAuth2RedirectError, OAuth2Error):
+ return self._create_error_result(
+ QueryStatus.FAILED, str(ex), start_time, query=query
+ )
+ except (OAuth2RedirectError, OAuth2Error) as ex:
# Let OAuth2 exceptions propagate so callers (MCP, API) can
# handle them with context-appropriate responses.
+ self._create_error_result(
+ QueryStatus.FAILED, str(ex), start_time, query=query
+ )
raise
Review Comment:
Rechecked this on 8698a1afe6. Failure recording catches metadata commit
errors and suppresses recovery-rollback errors; the OAuth handlers still
re-raise the original exception. All eight SQL/timeout/OAuth error and redirect
cases pass, including checks that callers receive the same OAuth exception
object when persistence fails.
##########
superset/sql/execution/executor.py:
##########
@@ -1008,14 +1020,24 @@ def _create_error_result(
:param start_time: Start time for calculating execution duration
:param partial_results: Optional list of StatementResult from
successful
statements before the failure
+ :param query: Persisted audit record, if execution reached that stage
:returns: QueryResult with error status
"""
from superset_core.queries.types import QueryResult as QueryResultType
+ if query is not None:
+ # A failed metadata flush must be rolled back before recording
failure.
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ query.status = status.value
Review Comment:
Rechecked and completed in 8698a1afe6. Error finalization uses
`commit_query_unless_stopped()`; the winning Stop retains its status,
timestamp, and cancellation metadata. The False branch now also closes its
transaction, and success publication uses the same guard. Separate SQLite
sessions reproduce Stop during failure finalization and during the final fetch.
--
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]
