rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4380811087
##########
superset/dashboards/api.py:
##########
@@ -2556,6 +2560,32 @@ def should_enqueue(cache_payload:
ScreenshotCachePayload) -> bool:
)
cache_payload = ScreenshotCachePayload(scope=cache_scope)
cache_payload.pending()
+ # A force-less staleness refresh of a still-valid capture
must
+ # not publish an imageless successor: carry the prior
+ # generation's last-good image onto this in-progress
payload so
+ # the read path keeps serving it while the refresh
renders, and
+ # a failed render retains it (ERROR keeps the image)
instead of
+ # leaving image_url at 404 for the error backoff. Any
valid,
+ # scope-matching retained image qualifies -- not only
UPDATED --
+ # so the image also survives a retry that rotates to a
fresh
+ # generation after a prior render failed (whose
predecessor is
+ # ERROR, still carrying the last-good bytes). The successor
+ # stays PENDING, so concurrent producers still coalesce
and the
+ # worker still recomputes it. Skipped for an explicit
force (the
+ # caller asked to discard the capture) and for a scope
mismatch
+ # (must never serve another object's image).
+ if (
+ not force
+ and cached_payload is not None
+ and cached_payload.get_scope() == cache_scope
Review Comment:
Addressed in
[970488566f](https://github.com/apache/superset/commit/970488566feb8e1fb3baad440ae9e64ec6c80772).
Added four producer cases using a real cache and the returned `image_url`:
valid wrong-scope and missing-scope predecessors, a correctly scoped UPDATED
predecessor without an image, and corrupt bytes. Each asserts 202, a correctly
scoped imageless Pending successor, 404 from its image URL, an unchanged
predecessor, and one force-less enqueue.
I also mutation-checked the guards: removing the scope conjunct makes both
scope cases fail (200 instead of 404), and removing the missing-image fallback
makes the scoped imageless case fail (500 instead of 202).
The broader review also caught a related retention gap: a rejected
replacement cache write could discard the previous valid image. The same commit
preserves it when the backend accepts the retained-image Error payload, with a
minimal imageless fallback when that also fails. Six worker regressions cover
retention, fallback rejection, wrong/missing scope, and invalid/absent prior
images. Final local validation: 147 unit tests, 48 API tests, all branch
pre-commit hooks, and mypy passed.
##########
superset/config.py:
##########
@@ -1414,6 +1414,13 @@ def sync_theme_logo_href(
# How long to treat a COMPUTING cache entry as an active lease before
considering
# the worker stuck. Should exceed the task soft_time_limit (300 s) by a
margin.
THUMBNAIL_COMPUTING_CACHE_TTL = int(timedelta(seconds=360).total_seconds())
+# How long a successfully-rendered (UPDATED) thumbnail stays servable before it
+# is recomputed. Cache backends without TTL eviction (e.g. S3) never expire
+# entries on their own, so without this a once-rendered thumbnail is served
+# forever even if it was a valid-but-blank capture. Defaults to match the
+# THUMBNAIL_CACHE_CONFIG CACHE_DEFAULT_TIMEOUT (7 days) so freshness matches
what
+# TTL-evicting backends already enforce. Set to 0 or None to disable (opt out).
+THUMBNAIL_UPDATED_CACHE_TTL = int(timedelta(days=7).total_seconds())
Review Comment:
Updated in
[970488566f](https://github.com/apache/superset/commit/970488566feb8e1fb3baad440ae9e64ec6c80772).
The config comment names the dashboard on-demand endpoint and dashboard
`compute-thumbnails` path, explicitly excludes chart screenshots and card-list
endpoints, and states that this is a refresh threshold rather than image
expiry. It also explains retained-image availability during refresh/error
backoff and that `0`/`None` disables this check without disabling backend
expiry or eviction. The seven-day value is described as a matching default, not
a live link to `CACHE_DEFAULT_TIMEOUT`.
--
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]
