sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4395661577
##########
superset/charts/api.py:
##########
@@ -1213,9 +1212,21 @@ def build_response(status_code: int) -> WerkzeugResponse:
task_status=cache_payload.get_status(),
)
- if cache_payload.should_trigger_task(force,
expected_scope=f"chart:{chart.id}"):
+ if cache_payload.should_trigger_task(
+ force,
+ expected_scope=f"chart:{chart.id}",
+ check_updated_staleness=screenshot_obj.supports_updated_staleness,
Review Comment:
The chart on-demand opt-in is not exercised by any added test:
`test_cache_screenshot_*` seeds misses or errors, and the stale-`UPDATED` tests
only hit the card `thumbnail` path. Changing this argument to `True` would
leave the suite green while `cache_screenshot` starts answering 202 and
enqueuing for old chart images the worker then skips. Could we add an API test
with a valid, matching-scope image aged past `THUMBNAIL_UPDATED_CACHE_TTL` that
requests `cache_screenshot` without force and asserts 200, no `delay` call, and
unchanged cached bytes?
##########
superset/utils/screenshots.py:
##########
@@ -191,32 +221,93 @@ def get_status(self) -> str:
return self.status.value
def get_invalid_image_reason(self) -> str | None:
- """Reason this payload's image should not be served/cached, or None if
- it passes validation (or it isn't claiming a successful screenshot)."""
- if self.status != StatusValues.UPDATED:
+ # Validate whenever an image is present, regardless of status: retained
+ # ERROR/COMPUTING entries still carry their previous image and are now
+ # served by the read paths, so a corrupt/blank retained image must be
+ # rejected (treated as a cache miss) too.
+ if self._image is None:
return None
return validate_screenshot_image(self._image)
+ def _age_seconds(self) -> float | None:
+ """Seconds since this entry's timestamp.
+
+ Timestamps are stored NAIVE (``datetime.now().isoformat()``), so the
age
+ is computed against a naive ``datetime.now()``. Keeping the naive
format
+ means old pods reading entries written by a newer pod (and vice versa)
+ during a rolling deploy never hit a naive-minus-aware ``TypeError``.
+
+ Corrupt or offset-aware timestamps return None so callers can recover.
+ Negative ages are preserved: clock/timezone skew must not expire error
+ backoff or computation leases on every request. Only the opt-in UPDATED
+ freshness check applies a bound to future timestamps.
+ """
+ try:
+ age = (
+ datetime.now() - datetime.fromisoformat(self.get_timestamp())
+ ).total_seconds()
+ except (ValueError, TypeError):
+ logger.warning(
+ "Unusable screenshot cache timestamp %r; "
+ "treating entry as expired/stale",
+ self.get_timestamp(),
+ )
+ return None
+ return age
+
def is_error_cache_ttl_expired(self) -> bool:
- error_cache_ttl = app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+ # strict '>' (an entry exactly at the TTL is still fresh). An unusable
+ # timestamp (age is None) is treated as expired so the entry
self-heals.
+ age_seconds = self._age_seconds()
return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() > error_cache_ttl
+ age_seconds is None or age_seconds >
app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+ )
def is_computing_stale(self) -> bool:
"""Check if a COMPUTING status is stale (task likely failed or
stuck)."""
return self.is_in_progress_stale()
def is_in_progress_stale(self, max_age_seconds: float | None = None) ->
bool:
"""Check if a pending or computing request has exceeded its lease."""
+ # '>=' (unlike the strict '>' of the ERROR/UPDATED helpers). An
unusable
+ # timestamp (age is None) is treated as stale so the entry self-heals.
computing_ttl = (
max_age_seconds
if max_age_seconds is not None
else app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
)
- return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() >= computing_ttl
+ age_seconds = self._age_seconds()
+ return age_seconds is None or age_seconds >= computing_ttl
+
+ def is_updated_stale(self) -> bool:
+ """Return whether an UPDATED entry needs refreshing when freshness is
enabled.
+
+ An unset/zero TTL disables the check. Unusable timestamps and future
+ timestamps beyond the skew tolerance are stale; bounded negative ages
+ remain fresh so web and worker clocks can converge.
+ """
+ # `.get` (not `[]` like the sibling ERROR/COMPUTING helpers) on
purpose:
+ # a deployment whose config predates this key should silently disable
the
+ # feature, not raise KeyError. Checked first so a disabled feature
never
+ # parses/logs an unusable timestamp.
+ updated_ttl = app.config.get("THUMBNAIL_UPDATED_CACHE_TTL")
+ if not updated_ttl: # None or 0 => disabled
+ return False
+ # strict '>' (an image exactly at the TTL is still fresh), matching
+ # is_error_cache_ttl_expired -- not the '>=' of is_computing_stale. An
+ # unusable timestamp (age is None) is treated as stale so it
self-heals.
+ age_seconds = self._age_seconds()
+ if (
+ age_seconds is not None
+ and age_seconds < -_FUTURE_TIMESTAMP_TOLERANCE_SECONDS
Review Comment:
A worker whose naive timestamp runs more than 300s ahead of the web host
(different `TZ` on the two containers, or a DST fall-back) makes every fresh
`UPDATED` entry look stale here. The web tier then rotates a new generation and
returns 202, the worker re-renders and writes another ahead-of-clock timestamp,
and the next force-less POST is stale again. A client polling until
`task_status` is `Updated` never converges, and each POST costs a full render.
Should a far-future `UPDATED` timestamp be treated as fresh (so it expires once
real age passes the TTL plus skew), or is skew beyond 300s intentionally
unsupported here?
--
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]
