rusackas commented on code in PR #40657:
URL: https://github.com/apache/superset/pull/40657#discussion_r3338265532
##########
superset/commands/report/execute.py:
##########
@@ -1031,17 +1031,45 @@ def next(self) -> None:
)
return
except Exception as ex:
- self.send_error(
- f"Error occurred for {self._report_schedule.type}:"
- f" {self._report_schedule.name}",
- str(ex),
- )
- self.update_report_schedule_and_log(
- ReportState.ERROR,
- error_message=REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER,
- )
+ # Ensure the schedule always transitions out of WORKING to
+ # ERROR, even if sending the error notification itself fails —
+ # otherwise the schedule is stuck in WORKING until the working
+ # timeout. Mirrors ReportNotTriggeredErrorState.next().
+ try:
+ self.send_error(
+ f"Error occurred for {self._report_schedule.type}:"
+ f" {self._report_schedule.name}",
+ str(ex),
+ )
+ except Exception: # noqa: BLE001 # pylint:
disable=broad-except
+ logger.warning(
+ "Failed to send error notification for report schedule
"
+ "(execution %s)",
+ self._execution_id,
+ exc_info=True,
+ )
+ finally:
+ try:
+ self.update_report_schedule_and_log(
+ ReportState.ERROR,
+
error_message=REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER,
+ )
+ except ReportScheduleUnexpectedError:
+ logger.warning(
+ "Failed to log ERROR state for report schedule "
+ "(execution %s) due to database issue",
+ self._execution_id,
+ exc_info=True,
+ )
Review Comment:
The marker is conditionally set: error_message starts as
REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER but gets overwritten with the send
exception message in the except block. The finally clause then records
whichever value error_message holds — so the marker only reaches
update_report_schedule_and_log when send_error() succeeds. This is the correct
behavior described in the PR.
##########
superset/commands/report/execute.py:
##########
@@ -1031,17 +1031,45 @@ def next(self) -> None:
)
return
except Exception as ex:
- self.send_error(
- f"Error occurred for {self._report_schedule.type}:"
- f" {self._report_schedule.name}",
- str(ex),
- )
- self.update_report_schedule_and_log(
- ReportState.ERROR,
- error_message=REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER,
- )
+ # Ensure the schedule always transitions out of WORKING to
+ # ERROR, even if sending the error notification itself fails —
+ # otherwise the schedule is stuck in WORKING until the working
+ # timeout. Mirrors ReportNotTriggeredErrorState.next().
+ try:
+ self.send_error(
+ f"Error occurred for {self._report_schedule.type}:"
+ f" {self._report_schedule.name}",
+ str(ex),
+ )
+ except Exception: # noqa: BLE001 # pylint:
disable=broad-except
+ logger.warning(
+ "Failed to send error notification for report schedule
"
+ "(execution %s)",
+ self._execution_id,
+ exc_info=True,
+ )
+ finally:
+ try:
+ self.update_report_schedule_and_log(
+ ReportState.ERROR,
+
error_message=REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER,
+ )
+ except ReportScheduleUnexpectedError:
+ logger.warning(
+ "Failed to log ERROR state for report schedule "
+ "(execution %s) due to database issue",
+ self._execution_id,
+ exc_info=True,
+ )
Review Comment:
Same as above — error_message is overwritten to str(send_ex) in the except
block if send_error() fails, so the marker is only recorded in the finally when
the notification was actually delivered. The code is correct; the pattern just
requires reading the try/except/finally flow carefully.
--
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]
