rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4136517250
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -5470,6 +5470,92 @@ def
test_cache_dashboard_screenshot_dashboard_not_found(self):
response = self._cache_screenshot(non_existent_id)
assert response.status_code == 404
+ @with_feature_flags(THUMBNAILS=True,
ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True)
+ @with_config({"THUMBNAIL_UPDATED_CACHE_TTL": 300})
+ @pytest.mark.usefixtures("create_dashboard_with_tag")
+ @patch("superset.dashboards.api.cache_dashboard_screenshot")
+ @patch("superset.dashboards.api.DashboardScreenshot.get_from_cache_key")
+ def test_cache_dashboard_screenshot_recomputes_stale_updated(
Review Comment:
Good catch — the test wasn't reaching the staleness branch. Without mocking
`get_current_api_generation_cache_key` the request had no prior generation
pointer, so `get_generation()` returned `(None, None)` and the endpoint took
the ordinary first-render/cache-miss branch (also a 202) — the mocked stale
payload was never consulted, and it would have stayed green even with the
`check_updated_staleness` wiring reverted.
Fixed in 92b4ba06: it now mocks `get_current_api_generation_cache_key` (plus
`set_current_api_generation_cache_key`) so the stale payload is genuinely
resolved and the staleness branch is exercised — dropping the wiring now
returns 200 and fails the test. Also switched the fixture to valid
PNG-signature bytes.
##########
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 92b4ba06. A force-less staleness refresh no longer publishes an
imageless successor generation: it now carries the prior generation's last-good
image onto the successor (new `ScreenshotCachePayload.retain_image`), so
`image_url` keeps serving it while the refresh renders and a failed render
retains it (`ERROR` keeps the image — `error()` without `discard_image`)
instead of 404ing for the error backoff. The successor stays `PENDING`, so
concurrent producers still coalesce and the worker's `should_trigger_task`
still recomputes it. Gated to force-less requests whose observed generation is
a scope-matching, valid `UPDATED` capture (an explicit `force` asked to discard
it; a scope mismatch must never serve another object's image).
Added an integration test
(`test_cache_dashboard_screenshot_stale_refresh_retains_last_good_image`)
asserting the successor carries the image, stays in-progress and remains
triggerable, plus a `retain_image` unit test.
One boundary worth calling out: this keeps the guarantee within a single
generation — if the refresh render fails and that `ERROR` generation's
`THUMBNAIL_ERROR_CACHE_TTL` then elapses, the next force-less request rotates
to a fresh generation whose predecessor is now `ERROR` (not `UPDATED`), so it
starts imageless again. Not a regression vs. the prior serve-stale behavior,
but happy to broaden the carry to `ERROR`-with-valid-image predecessors for
full multi-hop coverage if you'd prefer.
--
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]
