sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4277897326
##########
superset/dashboards/api.py:
##########
@@ -2535,6 +2539,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
Review Comment:
The `not force` gate is what keeps an explicit force from inheriting the
previous capture (the successor would otherwise serve the old bytes at
`image_url` while the forced render is pending), but nothing pins it. The new
carry tests are all force-less, and the existing force test seeds `b"old
image"`, which fails image validation and so never reaches this branch;
removing `not force` leaves the suite green. Could we add an API test with a
valid PNG, matching-scope predecessor and `force=True` that asserts 202, one
queued task, and an imageless successor payload?
##########
superset/utils/screenshots.py:
##########
@@ -131,14 +139,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:
Keeping the stored `ERROR` status here means an image-bearing `ERROR` entry
(a failed refresh that retained its last-good capture) now reaches the chart
card `thumbnail` handler's expired-error retry branch, which does
`cache.set(cache_key, ScreenshotCachePayload().to_dict())` before enqueueing.
Before this change that entry rehydrated as `UPDATED` and was served. So after
the error backoff elapses, the first card-list load wipes the retained image,
and if the render fails again every later load is a 404 for another backoff
window. The on-demand handler now preserves the image across this retry; should
the card path do the same, or skip the empty pre-write when the entry still
holds a valid image?
--
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]
