sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r3858605757
##########
superset/utils/screenshots.py:
##########
@@ -350,7 +398,9 @@ def compute_and_cache( # pylint: disable=too-many-arguments
self.get_from_cache_key(cache_key) or
ScreenshotCachePayload()
)
if not cache_payload.should_trigger_task(
- force=force, expected_scope=self.cache_scope
+ force=force,
+ expected_scope=self.cache_scope,
+ check_updated_staleness=self.supports_updated_staleness,
):
Review Comment:
After a stale dashboard refresh fails, deserializing the cached entry turns
its stored `ERROR` (or `COMPUTING`) status back into `UPDATED` because the old
image is still present. The next request can then launch another refresh
immediately instead of honoring the error/lease TTL, so a broken renderer can
be retried on every request. Should the payload preserve the stored status
while retaining the previous image?
##########
superset/dashboards/api.py:
##########
@@ -1996,7 +1996,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 new opt-in is wired only by this endpoint, but the added tests never
exercise it; removing this argument still leaves the new tests green. Could we
add an API test with a stale, matching-scope payload that asserts a force-less
request schedules the refresh and returns 202?
##########
superset/dashboards/api.py:
##########
@@ -1996,7 +1996,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:
Agreed—the new stale branch leaves the cache as `UPDATED` until the task
starts, so concurrent requests can all enqueue work before the lock is
acquired. Could this endpoint claim the refresh before scheduling so only one
task is queued?
--
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]
