rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r3978552129
##########
superset/utils/screenshots.py:
##########
@@ -183,21 +191,66 @@ def get_invalid_image_reason(self) -> str | None:
return None
return validate_screenshot_image(self._image)
+ def _age_seconds(self) -> float | None:
+ """Seconds since this entry's timestamp, or None if the stored
+ timestamp is unusable -- a corrupt string (ValueError) or a legacy
+ tz-aware value that cannot be subtracted from naive now() (TypeError).
+ Callers treat None as 'past any TTL' so the entry self-heals."""
+ try:
+ return (
+ 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
+
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)."""
- computing_ttl = app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
+ # '>=' (unlike the strict '>' of the ERROR/UPDATED helpers). An
unusable
+ # timestamp (age is None) is treated as stale so the entry self-heals.
+ age_seconds = self._age_seconds()
return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() >= computing_ttl
+ age_seconds is None
+ or age_seconds >= app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
+ )
+
+ def is_updated_stale(self) -> bool:
+ """Whether a successfully-rendered (UPDATED) entry is old enough to be
+ recomputed. Returns False when the TTL is unset/0 (no-op unless an
operator
+ opts in). A timestamp we cannot use -- a corrupt string (ValueError)
or a
+ legacy tz-aware string that parses but cannot be subtracted from naive
+ now() (TypeError) -- is logged and treated as stale so it self-heals
rather
+ than being served forever."""
+ # `.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()
+ return age_seconds is None or age_seconds > updated_ttl
Review Comment:
Fixed — timestamps are now written and compared in UTC, and a negative age
(worker-ahead skew) is treated as fresh rather than stale, so the web tier and
worker converge instead of looping at 202. Legacy naive timestamps are assumed
UTC; added a worker-ahead convergence test.
##########
superset/utils/screenshots.py:
##########
@@ -117,14 +117,22 @@ def __init__(
@classmethod
def from_dict(cls, payload: ScreenshotCachePayloadType) ->
ScreenshotCachePayload:
- return cls(
+ instance = cls(
image=base64.b64decode(payload["image"]) if payload["image"] else
None,
status=StatusValues(payload["status"]),
timestamp=payload["timestamp"],
# `.get` rather than `payload["scope"]`: entries cached before this
# field existed won't have the key.
scope=payload.get("scope"),
)
+ # `__init__` infers UPDATED whenever an image is present -- convenient
for
+ # the `ScreenshotCachePayload(image=bytes)` and legacy
bytes-reconstruction
+ # paths, but wrong when rehydrating a persisted entry: an ERROR or
COMPUTING
+ # entry keeps its previous image, and re-inferring UPDATED here would
mask it
+ # as fresh and bypass the shorter ERROR/COMPUTING recovery TTLs.
Restore the
+ # persisted status explicitly.
+ instance.status = StatusValues(payload["status"])
Review Comment:
Fixed — the chart `/screenshot/<digest>/` reader now serves the retained
image whenever present (mirroring the dashboard reader) instead of gating on
`UPDATED`, so a failed forced refresh returns 200 during the error backoff, not
404. Added a test.
##########
superset/dashboards/api.py:
##########
@@ -2182,6 +2184,8 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any)
-> WerkzeugResponse:
"DashboardRestApi.thumbnail", pk=dashboard.id, digest=cache_key
)
+ # No check_updated_staleness here on purpose: this high-traffic
card-list
Review Comment:
Added `test_thumbnail_does_not_recompute_stale_updated` for the dashboard
card path: a stale `UPDATED` entry serves 200 + cached bytes with no
`cache_dashboard_thumbnail.delay`. Since the dashboard flag is `True`, it fails
if the flag is propagated into this path.
##########
superset/charts/api.py:
##########
@@ -1355,6 +1359,8 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any)
-> WerkzeugResponse:
screenshot_obj.get_from_cache_key(cache_key) or
ScreenshotCachePayload()
)
+ # No check_updated_staleness here on purpose: this high-traffic
card-list
Review Comment:
Good catch — strengthened it: the test now forces
`ChartScreenshot.supports_updated_staleness=True` (with a 300s TTL), so it goes
red if the endpoint copies
`check_updated_staleness=screenshot_obj.supports_updated_staleness`.
##########
superset/dashboards/api.py:
##########
@@ -1983,7 +1983,9 @@ def build_response(status_code: int) -> WerkzeugResponse:
)
if cache_payload.should_trigger_task(
- force, expected_scope=f"dashboard:{dashboard.id}"
+ force,
+ expected_scope=f"dashboard:{dashboard.id}",
+ check_updated_staleness=screenshot_obj.supports_updated_staleness,
Review Comment:
Done — the `UPDATING.md` bullet now carries the worker-first rollout note.
--
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]
