sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r3977548458
##########
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:
Restoring the persisted `ERROR` status makes the chart screenshot reader
reject the retained last-good image, while the fresh error backoff suppresses
another task, so a failed forced refresh leaves the returned `image_url` at 404
for up to a day. Could the chart read path serve the retained image or
otherwise keep the 200/task contract consistent during the backoff?
##########
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:
`DashboardScreenshot` opts into updated staleness globally, so copying the
new flag into this high-traffic card path would turn every stale card request
into 202 plus a queued render, but no endpoint test pins this exclusion. Could
we add a stale-`UPDATED` dashboard thumbnail test asserting 200 with the cached
bytes and no `cache_dashboard_thumbnail.delay`?
##########
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:
This test still passes if this endpoint copies
`check_updated_staleness=screenshot_obj.supports_updated_staleness`, because
`ChartScreenshot` currently leaves that flag false. Could the test force the
flag true (or assert the kwarg is absent) so it actually protects the
endpoint-level exclusion?
##########
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:
The current `UPDATING.md` entry still does not mention worker-first rollout,
so a new web pod can return 202 while an old worker skips the refresh until the
worker pool rolls. Could we add the promised rollout requirement or carry
refresh intent in the task?
--
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]
