rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4198053172
##########
superset/charts/api.py:
##########
@@ -1274,16 +1288,23 @@ 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 validates whatever image is present
regardless of
Review Comment:
Good catch — fixed in a4ad132. The worker (`compute_and_cache`) now discards
any retained image when it rebinds a cache entry to a *different* scope (the
scope-mismatch branch of `should_trigger_task`): it starts the rebind from a
clean, imageless payload before `set_scope`/`computing()`, so the previous
object's bytes can never be served under the new object's authorized
`screenshot` URL during the COMPUTING/ERROR window. Legitimate same-scope
staleness carries are unaffected. Added a unit test
(`test_mismatched_scope_discards_prior_image_on_failed_recompute`).
##########
superset/dashboards/api.py:
##########
@@ -2046,6 +2046,10 @@ def should_enqueue(cache_payload:
ScreenshotCachePayload) -> bool:
force,
expected_scope=cache_scope,
force_retry_after_seconds=SCREENSHOT_API_FORCE_RETRY_SECONDS,
+ # Only DashboardScreenshot opts in: a stale-but-valid UPDATED
+ # generation older than THUMBNAIL_UPDATED_CACHE_TTL enqueues a
+ # refresh while the old generation stays servable.
+
check_updated_staleness=screenshot_obj.supports_updated_staleness,
Review Comment:
Fixed in a4ad132 — both halves:
- **Retry rotation:** the endpoint carry gate no longer requires the
predecessor to be `UPDATED`. Any valid, scope-matching retained image carries
forward, so a retry that rotates to a fresh generation after a failed render
(predecessor now `ERROR`, still holding the last-good bytes) keeps serving it
instead of 404ing.
- **Enqueue failure:** `mark_cache_error_if_incomplete` now preserves a
valid *same-scope* image instead of unconditionally discarding, so a failed
broker publish doesn't drop the carried capture.
Both still discard invalid or cross-scope bytes — the unifying rule is
*retain only when valid **and** same-scope, discard on any scope change*, which
is also the fix for your `charts/api.py` comment. Added tests for the rotation
carry and the `mark_cache_error_if_incomplete` retain/drop behavior.
--
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]
