sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4198830583
##########
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:
Restoring the stored `ERROR` status here means a failed forced refresh
leaves an `ERROR` entry that still carries that chart's image and scope, and
`should_trigger_task` only treats a scope mismatch as a cache miss for
`UPDATED`. A second chart with identical params (the chart digest ignores the
chart id, so a "save as" copy shares the key) that calls `cache_screenshot`
then gets 200 with no task queued, while its `image_url` 404s on the scope
check until the error backoff expires (a day by default). Before this change
the image-bearing entry rehydrated as `UPDATED`, so the scope mismatch
re-triggered a render. Should the scope-mismatch clause apply to any status so
a new owner isn't held behind another object's error backoff?
##########
tests/unit_tests/utils/screenshot_test.py:
##########
@@ -526,6 +577,30 @@ def test_recomputes_updated_entry_with_mismatched_scope(
assert cache_payload["image"] != b"initial_value"
assert cache_payload["scope"] == "dashboard:5"
+ def test_mismatched_scope_discards_prior_image_on_failed_recompute(
Review Comment:
This only covers the mismatched-scope branch of the new reset in
`compute_and_cache`. Nothing runs the worker for a same-scope entry whose
render fails (the retry-carry API tests mock the worker), so changing the reset
to apply to every scoped payload would drop the last-good image on a failed
refresh while every test here stays green. Could we add a case that seeds a
matching-scope payload holding `FAKE_PNG_BYTES`, makes `get_screenshot` raise,
and asserts the stored payload is `Error` with the original image bytes still
present?
--
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]
