EnxDev commented on code in PR #44336:
URL: https://github.com/apache/superset/pull/44336#discussion_r4060355380
##########
superset/commands/report/execute.py:
##########
@@ -142,26 +150,49 @@ def resolve_executor_user(model: ReportSchedule) ->
tuple["User", str]:
def _should_build_execution_context(model: ReportSchedule) -> bool:
- """
- Whether an execution should run under a :class:`ReportExecutionContext`.
-
- Reports always do — their behavior is unchanged. Alerts join them only when
- they deliver a rendered PNG/PDF screenshot to recipients, which happens
when
- ``ALERTS_ATTACH_REPORTS`` is enabled. Delivered screenshots must fail
closed:
- the context selects the fail-closed readiness predicate and disables
- partial-tile fallback, so a blank or incomplete capture raises instead of
- being delivered.
-
- CSV/text alerts, alerts without the attach flag, the non-delivered
- query-context capture, and UI thumbnails are deliberately excluded and keep
- their lenient capture contract.
- """
+ """Give every scheduled report and alert a shared deadline and ownership
context."""
+ return model.type in (ReportScheduleType.REPORT, ReportScheduleType.ALERT)
+
+
+def _uses_report_capture_contract(model: ReportSchedule) -> bool:
+ """Keep ownership separate from the existing rendered-alert capture
policy."""
+ return model.type == ReportScheduleType.REPORT or (
+ model.report_format in (ReportDataFormat.PNG, ReportDataFormat.PDF)
+ and feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS")
+ )
+
+
+def _execution_budget_seconds(model: ReportSchedule) -> float:
+ """Preserve alert Celery limits rather than applying the global report
budget."""
if model.type == ReportScheduleType.REPORT:
- return True
- return model.report_format in (
- ReportDataFormat.PNG,
- ReportDataFormat.PDF,
- ) and feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS")
+ return resolve_report_execution_budget_seconds(
+ app.config, working_timeout=model.working_timeout
+ )
+ return float(
+ get_report_task_timeout_options(
+ is_report=False, working_timeout=model.working_timeout,
config=app.config
+ ).get("soft_time_limit", float("inf"))
+ )
Review Comment:
Could we translate the unlimited deadline back to `None` before passing it
to the HTTP transport? With `ALERT_REPORTS_WORKING_TIME_OUT_KILL=False` (or no
`working_timeout`) and `ALERT_REPORTS_CSV_REQUEST_TIMEOUT=None`, this creates
an infinite deadline. `_phase_timeout()` then returns `inf`, which reaches
`urllib` and fails in `socket.settimeout()` with `OverflowError: timestamp out
of range for platform time_t`, before the request is sent.
I reproduced this through `_get_data(CSV)` against a local HTTP server: it
succeeds without the execution context and fails with the new context. CSV/XLSX
and embedded-text alerts share this timeout path. `None` is a documented value
for the request timeout, so preserving `None` when both limits are unbounded
would keep those configurations working. A transport test for that combination
would catch what the deadline-only tests miss.
--
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]
