rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4025759063
##########
superset/utils/screenshots.py:
##########
@@ -111,20 +111,28 @@ def __init__(
scope: str | None = None,
):
self._image = image
- self._timestamp = timestamp or datetime.now().isoformat()
+ self._timestamp = timestamp or datetime.now(timezone.utc).isoformat()
Review Comment:
Reverted the UTC-aware timestamps back to naive, so the serialized format
stays backward-compatible — mixed-version pods during a rolling deploy no
longer hit a naive−aware `TypeError`. Per your earlier "normalize to UTC or
tolerate bounded skew" note I went with the bounded-tolerance route (a small
negative age is treated as fresh so web/worker converge; only an
implausibly-far-future timestamp self-heals).
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -4259,6 +4259,92 @@ def
test_cache_dashboard_screenshot_dashboard_not_found(self):
response = self._cache_screenshot(non_existent_id)
assert response.status_code == 404
+ @with_feature_flags(THUMBNAILS=True,
ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
+ @with_config({"THUMBNAIL_UPDATED_CACHE_TTL": 300})
+ @pytest.mark.usefixtures("create_dashboard_with_tag")
+ @patch("superset.dashboards.api.cache_dashboard_screenshot")
+ @patch("superset.dashboards.api.DashboardScreenshot.get_from_cache_key")
+ def test_cache_dashboard_screenshot_recomputes_stale_updated(
+ self, mock_get_from_cache_key, mock_cache_task
+ ):
+ """A force-less request whose cached UPDATED entry is older than
+ THUMBNAIL_UPDATED_CACHE_TTL -- but still valid and correctly scoped --
+ must reschedule the Celery task. This exercises the endpoint's
+ ``check_updated_staleness=screenshot_obj.supports_updated_staleness``
+ wiring (True only for dashboards); dropping that argument makes the
+ endpoint serve the stale entry (200) instead, failing this test."""
+ from datetime import datetime, timedelta
+
+ self.login(ADMIN_USERNAME)
+
+ dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "dash with tag")
+ .first()
+ )
+ # A valid, correctly-scoped UPDATED entry, but 400s old against a 300s
TTL.
+ stale_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
Review Comment:
Reverted the fixtures to naive `datetime.now()` — with naive comparison a
400s-old entry is unambiguously stale regardless of host timezone.
##########
superset/charts/api.py:
##########
@@ -1276,16 +1279,22 @@ def screenshot(self, pk: int, digest: str) ->
WerkzeugResponse:
# serve its image under a different, merely-accessible `pk`.
if cache_payload.get_scope() != f"chart:{chart.id}":
return self.response_404()
- if cache_payload.status == StatusValues.UPDATED:
- try:
- image = cache_payload.get_image()
- except ScreenshotImageNotAvailableException:
- return self.response_404()
- return Response(
- FileWrapper(image),
- mimetype="image/png",
- direct_passthrough=True,
- )
+ # Serve whenever a valid image is present instead of gating on
+ # status == UPDATED. A failed forced refresh leaves the entry in an
+ # ERROR/COMPUTING backoff while still carrying the retained
last-good
+ # image; requiring UPDATED here would 404 that image for up to a
day.
+ # get_from_cache_key already rejects an invalid UPDATED image, and
a
Review Comment:
`get_invalid_image_reason` now validates whenever an image is present (not
only `UPDATED`), so a corrupt/blank retained `ERROR`/`COMPUTING` image is
treated as a cache miss rather than served as `image/png`. Added a
non-`UPDATED` invalid-image test.
##########
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:
Dropped the "assume legacy-naive = UTC" step — timestamps are naive again
with a bounded negative-skew tolerance, so an upgrade on a UTC-ahead host no
longer turns a recent entry into a future one (no timezone-offset 404).
##########
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:
The chart on-demand retry now marks the entry `COMPUTING` (preserving the
retained image) instead of writing an empty `PENDING`, so `image_url` keeps
serving the last-good image across a force-fail → retry-fail. Added a test
asserting the pre-write keeps the image.
##########
docs/admin_docs/configuration/cache.mdx:
##########
@@ -326,6 +326,32 @@ Then on configuration:
WEBDRIVER_AUTH_FUNC = auth_driver
```
+### Refreshing successfully-rendered thumbnails
+
+Once a thumbnail renders successfully it is cached with no expiry, so on cache
backends without
+TTL eviction (for example S3) a stale — or valid-but-blank — capture can be
served indefinitely to
+callers that do not pass `force=true`. `THUMBNAIL_UPDATED_CACHE_TTL` bounds
how long a
+successfully-rendered thumbnail is served before it is recomputed:
+
+```python
+from datetime import timedelta
+
+# Recompute a successfully-rendered thumbnail once it is older than 7 days
(the default,
+# matching THUMBNAIL_CACHE_CONFIG's CACHE_DEFAULT_TIMEOUT).
+THUMBNAIL_UPDATED_CACHE_TTL = int(timedelta(days=7).total_seconds())
+```
+
+- **Opt out:** set `THUMBNAIL_UPDATED_CACHE_TTL = 0` (or `None`) to keep
serving a rendered
Review Comment:
Fixed — the doc now says `0`/`None` disables the new freshness check and
falls back to the cache backend's own retention (`THUMBNAIL_CACHE_CONFIG`'s
`CACHE_DEFAULT_TIMEOUT`, 7 days by default), which only means "indefinitely" on
a no-eviction backend like S3.
--
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]
