sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4129121314
##########
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:
This test doesn't exercise the staleness path it names:
`get_current_api_generation_cache_key` isn't mocked, and this dashboard has no
prior generation pointer, so the endpoint never reaches `get_from_cache_key` at
all -- the mocked stale payload is unused and the 202 comes from the ordinary
first-render/cache-miss branch instead. Reverting the `check_updated_staleness`
wiring on this endpoint wouldn't fail this test. Could it seed an existing
generation (for example via `set_current_api_generation_cache_key`) so the
assertions actually pin the stale-refresh behavior?
##########
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:
Opting this endpoint into `check_updated_staleness` means an ordinary
force-less request -- not only an explicit `force=true` one -- can now publish
a fresh, imageless generation pointer before the refresh renders. If that
render fails, the current generation lands in `Error` with no image (a
brand-new payload never carried one), so a caller following the response's
`image_url`/status sees no image until the error backoff clears, instead of
continuing to see the previous, still-valid capture as it would have before
this change. Should a TTL-triggered refresh keep the prior generation current
until its replacement succeeds?
--
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]
