eschutho commented on PR #42273:
URL: https://github.com/apache/superset/pull/42273#issuecomment-5146132269
> Does a report on a retry-enabled schedule get another attempt after this
raises, or does one flaky tiled capture now fail the whole run?
Good question — traced it through the execution path. Short answer: **the
current run fails, but the schedule's next cron occurrence runs completely
fresh, and nothing about this raise changes existing retry semantics — because
there were none for captures to begin with.**
Details, with the relevant code:
- **Within a run, screenshot capture has never had a retry** — before or
after this PR. `_get_screenshots`
(`superset/commands/report/execute.py:548-635`) makes one capture attempt per
screenshot and wraps any exception as `ReportScheduleScreenshotFailedError`.
The raise this PR adds is caught by that same `except Exception` wrapper, so it
flows through exactly the same channel as every other capture failure (element
never appearing, browser crash, etc.).
- **The `reports.execute` Celery task has no autoretry**
(`superset/tasks/scheduler.py:117` — plain
`@celery_app.task(name="reports.execute", bind=True)`; the
`autoretry_for=(Exception,)` config a few lines above it belongs to the beat
`reports.scheduler` task, which only *enqueues* work). The one retry knob in
this area, `ALERT_REPORTS_QUERY_EXECUTION_MAX_TRIES`, applies to alert SQL
evaluation in `AlertCommand` (`superset/commands/report/alert.py:256`), not to
captures — so no configured retry is being bypassed.
- **A failed run does not wedge the schedule.** ERROR is an initial state in
the state machine (`ReportNotTriggeredErrorState.current_states =
[ReportState.NOOP, ReportState.ERROR]`, `initial = True` —
`execute.py:1189-1200`), so the next cron tick starts a brand-new WORKING run
and the flaky capture gets its retry then. Repeat error *notifications* are
throttled by the schedule's `grace_period` (`is_in_error_grace_period`), so a
flaky capture on a frequent schedule doesn't spam owners.
So the trade this PR makes is precisely: **before**, one flaky tiled capture
silently delivered an unguarded (often blank/spinner) screenshot and recorded
the run as SUCCESS; **after**, that run records ERROR with an owner
notification, and the next scheduled occurrence retries fresh. If we want an
*in-run* capture retry on top of that, it's a reasonable follow-up, but it's
orthogonal to removing the unsafe fallback — and worth noting #42624 explicitly
considered and deferred in-run retry ("retrying delivery is not safe without
provider idempotency"; a capture-only retry wouldn't have that problem, but
belongs in its own PR).
--
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]
