amaannawab923 commented on code in PR #42481:
URL: https://github.com/apache/superset/pull/42481#discussion_r3675056440
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,101 @@ def is_in_error_grace_period(self) -> bool:
< last_success.end_dttm
)
+ def _get_retry_delay(self, attempt: int) -> int:
+ """Exponential backoff: base * 2^attempt, capped at a configurable
max."""
+ base: int = app.config.get("ALERT_REPORTS_RETRY_BASE_DELAY_SECONDS",
60)
+ cap: int = app.config.get("ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS",
3600)
+ return min(base * (2**attempt), cap)
+
+ def _is_retry_window_stale(self) -> bool:
+ """
+ Return True if a new crontab window fired while the previous one was
+ still retrying. When stale the retry counter must be reset so the new
+ window gets a fresh retry budget.
+ """
+ anchor = self._report_schedule.retry_scheduled_dttm
+ return anchor is not None and anchor != self._scheduled_dttm
Review Comment:
i think this stale check might never work the way it's meant to.
`_scheduled_dttm` is tz-aware in prod (celery eta / the
`datetime.now(tz=timezone.utc)` fallback), but `retry_scheduled_dttm` is a
plain `DateTime` column so once it's saved and read back on the next retry it
comes back naive. aware != naive is always true in python, so
`_is_retry_window_stale()` returns true on every retry, resets the counter each
time, and `retry_attempt` just bounces 0 -> 1 -> 0 forever. so it never hits
`max_attempts`, never errors out, and never sends the retry emails (that branch
needs `current_attempt > 0`). basically it retries forever at a fixed interval
instead of giving up after N.
`find_active()` already deals with this same column by doing
`.replace(tzinfo=None)` to match the naive storage, so probably just normalize
both sides here too (or compare the iso string / a window token instead of raw
datetimes). heads up that the tests pass naive `datetime.utcnow()` so this path
doesn't really get exercised, a case with a tz-aware scheduled_dttm that saves
+ reloads the schedule would catch it.
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,101 @@ def is_in_error_grace_period(self) -> bool:
< last_success.end_dttm
)
+ def _get_retry_delay(self, attempt: int) -> int:
+ """Exponential backoff: base * 2^attempt, capped at a configurable
max."""
+ base: int = app.config.get("ALERT_REPORTS_RETRY_BASE_DELAY_SECONDS",
60)
+ cap: int = app.config.get("ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS",
3600)
+ return min(base * (2**attempt), cap)
+
+ def _is_retry_window_stale(self) -> bool:
+ """
+ Return True if a new crontab window fired while the previous one was
+ still retrying. When stale the retry counter must be reset so the new
+ window gets a fresh retry budget.
+ """
+ anchor = self._report_schedule.retry_scheduled_dttm
+ return anchor is not None and anchor != self._scheduled_dttm
+
+ def _increment_retry(self) -> int:
+ """Increment retry_attempt, set the window anchor, and return the new
count."""
+ self._report_schedule.retry_attempt += 1
+ self._report_schedule.retry_scheduled_dttm = self._scheduled_dttm
+ return self._report_schedule.retry_attempt
+
+ def _reset_retry_counter(self) -> None:
+ """Reset retry state after a terminal outcome."""
+ self._report_schedule.retry_attempt = 0
+ self._report_schedule.retry_scheduled_dttm = None
+
+ def _schedule_retry(self, delay_seconds: int) -> None:
+ """Re-queue the execute task with the given countdown (seconds)."""
+ # Lazy import to avoid a circular dependency between execute.py and
scheduler.py
+ from superset.tasks.scheduler import execute as execute_task # noqa:
PLC0415
+
+ # Pass the original crontab-trigger timestamp so the retry task
+ # shares the same window identity. This lets _is_retry_window_stale()
+ # detect when a *new* crontab window fires while retries are in-flight.
+ execute_task.apply_async(
+ (self._report_schedule.id, self._scheduled_dttm.isoformat()),
+ countdown=delay_seconds,
+ )
+
+ def send_retry_notification(
+ self, attempt: int, max_attempts: int, error_message: str
+ ) -> None:
+ """
+ Send a per-retry-attempt failure notification to the owners and/or
recipients
+ selected via the retry_notify_owners / retry_notify_recipients flags.
+ """
+ recipients: list[ReportRecipients] = []
+
+ if self._report_schedule.retry_notify_owners:
+ recipients.extend(
+ [
+ ReportRecipients(
+ type=ReportRecipientType.EMAIL,
+ recipient_config_json=json.dumps({"target":
s.user.email}),
+ )
+ for s in self._report_schedule.editors
+ if s.type == SubjectType.USER and s.user
+ ]
+ )
+
+ if self._report_schedule.retry_notify_recipients:
+ recipients.extend(self._report_schedule.recipients)
+
+ if not recipients:
+ return
+
+ header_data = self._get_log_data()
+ url = self._get_url(user_friendly=True)
+ notification_content = NotificationContent(
+ name=sanitize_title(self._report_schedule.name),
+ text=error_message,
+ header_data=header_data,
+ url=url,
+ retry_attempt=attempt,
+ retry_max_attempts=max_attempts,
+ )
+ self._send(notification_content, recipients)
+
+ def send_final_failure_report(self, error_message: str) -> None:
Review Comment:
quick question on intent here. the description says `send_failed_reports`
will "deliver the report to recipients even after all retries fail", but this
builds a `NotificationContent` with just `text=error_message` and no
screenshots/csv/embedded data, so recipients get a failure notice with the
error rather than the actual report. if that's what you meant then all good and
maybe just reword the option, but if the idea was to send the last-attempt
report itself then this doesn't quite do that yet.
##########
superset/commands/report/execute.py:
##########
@@ -1224,6 +1321,70 @@ def next(self) -> None: # noqa: C901
if isinstance(first_ex, SupersetErrorsException):
error_message = ";".join([error.message for error in
first_ex.errors])
+ # --- Retry logic ---
+ retry_on_failure: bool = self._report_schedule.retry_on_failure
+ max_attempts: int = self._report_schedule.retry_max_attempts
+
+ # If a new crontab window has fired since the first failure, reset
the
+ # retry counter so this window gets a fresh budget.
+ if retry_on_failure and self._is_retry_window_stale():
+ self._reset_retry_counter()
+
+ current_attempt = self._report_schedule.retry_attempt
+
+ # If this execution was itself a retry (current_attempt > 0),
+ # send the retry-failure notification *after* the attempt ran so
+ # the email reflects what happened, not what is about to be
+ # scheduled. ("You will receive an update after each retry.")
+ if retry_on_failure and current_attempt > 0:
+ try:
+ self.send_retry_notification(
+ current_attempt, max_attempts, error_message
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.warning(
+ "Failed to send retry notification for report schedule
"
+ "(execution %s)",
+ self._execution_id,
+ exc_info=True,
+ )
+
+ if retry_on_failure and current_attempt < max_attempts:
+ # Schedule another attempt and exit cleanly (don't re-raise).
+ self._increment_retry()
+ try:
+ self.update_report_schedule_and_log(
+ ReportState.RETRYING, error_message=error_message
+ )
+ except ReportScheduleUnexpectedError as logging_ex:
+ logger.warning(
+ "Failed to log RETRYING state for report schedule "
+ "(execution %s) due to database issue",
+ self._execution_id,
+ exc_info=True,
+ )
+ raise first_ex from logging_ex
+ self._schedule_retry(
Review Comment:
small one, the backoff skips its first step. this runs after
`_increment_retry()`, so `retry_attempt` is already >= 1 and `_get_retry_delay`
gives `base * 2**1` at minimum, the `base * 2**0` case never happens. so the
first retry waits 120s instead of 60s with the defaults. if you want it to
actually start at `base`, computing the delay before the increment (or with
`attempt - 1`) would do it.
##########
superset/commands/report/execute.py:
##########
@@ -1165,6 +1165,101 @@ def is_in_error_grace_period(self) -> bool:
< last_success.end_dttm
)
+ def _get_retry_delay(self, attempt: int) -> int:
+ """Exponential backoff: base * 2^attempt, capped at a configurable
max."""
+ base: int = app.config.get("ALERT_REPORTS_RETRY_BASE_DELAY_SECONDS",
60)
Review Comment:
`ALERT_REPORTS_RETRY_BASE_DELAY_SECONDS` and
`ALERT_REPORTS_RETRY_MAX_DELAY_SECONDS` are read here (and in `daos/report.py`)
but they're not declared in `config.py`, so operators can't really discover
them. might be worth adding them there with the defaults and a one-line comment
so they're documented alongside the other `ALERT_REPORTS_*` settings.
--
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]
