geido commented on code in PR #44144:
URL: https://github.com/apache/superset/pull/44144#discussion_r3988569172
##########
superset/dashboards/api.py:
##########
@@ -1923,6 +1923,8 @@ def cache_dashboard_screenshot(self, pk: int, **kwargs:
Any) -> WerkzeugResponse
$ref: '#/components/responses/404'
500:
$ref: '#/components/responses/500'
+ 503:
Review Comment:
Addressed in the latest push. `superset update-api-docs` on the exact
rebased source now produces no diff; the committed spec includes request
`permalinkKey`, response `permalink_key`, the existing-task 200 response, and
the cache-failure 503 response.
##########
superset/utils/screenshot_utils.py:
##########
@@ -382,11 +387,13 @@ def _unready_chart_holders_js_body(*, viewport_only:
bool) -> str:
}});
const hasUnpaintedAgGrid = unpaintedAgGrids.length > 0;
// Ready = a settled error/empty/missing state, or a slice container
- // whose renderer has painted. ECharts and AG Grid expose explicit
- // completion signals; keep either host unready until its signal fires.
+ // whose plugin loaded and rendered. Canvas/grid/map renderers expose
+ // additional paint signals; keep their hosts unready until those fire.
const isReady = !stillLoading && (
hasErrorOrEmpty || (
- hasSliceContainer && !hasUnpaintedEchart && !hasUnpaintedAgGrid
+ hasSliceContainer && hasRenderedChart
+ && !hasUnpaintedEchart && !hasUnpaintedAgGrid
+ && !hasUnpaintedDeckGl
Review Comment:
Addressed. The rendered/async/stable-ready and blank-image gates are enabled
only when `require_complete_capture=True`; only the dashboard API screenshot
worker opts into that mode. Scheduled reports and ordinary thumbnail captures
retain their prior readiness predicates, with regression coverage for the
default path.
##########
superset-frontend/plugins/preset-chart-deckgl/src/DeckGLContainer.tsx:
##########
@@ -179,6 +194,7 @@ export const DeckGLContainer = memo(
<MapLibreMap
{...viewState}
onMove={onMove}
+ onIdle={onMapIdle}
Review Comment:
Addressed and tested fail-closed. Deck readiness is scoped to the current
source generation and requires MapLibre idle plus a Deck `onAfterRender`;
MapLibre/style/source and Deck errors are tracked and cannot mark the host
painted. A Docker capture with explicit OSM tiles reached Updated with painted
map/layers, while an unreachable tile source reached terminal Error and GET
returned 404, so a 401/no-egress case cannot cache a blank map.
##########
superset/dashboards/api.py:
##########
@@ -1983,26 +1988,51 @@ def build_response(status_code: int) ->
WerkzeugResponse:
task_status=cache_payload.get_status(),
)
- if cache_payload.should_trigger_task(
- force, expected_scope=f"dashboard:{dashboard.id}"
+ if cached_payload is None or cache_payload.should_enqueue_task(
+ force, expected_scope=cache_scope
):
logger.info("Triggering screenshot ASYNC")
- cache_dashboard_screenshot.delay(
- username=get_current_user(),
- guest_token=(
- g.user.guest_token
- if get_current_user() and isinstance(g.user, GuestUser)
- else None
- ),
- dashboard_id=dashboard.id,
- dashboard_url=dashboard_url,
- thumb_size=thumb_size,
- window_size=window_size,
- cache_key=cache_key,
- force=force,
- )
+ cache_payload.pending()
Review Comment:
Addressed in the UI flow. It now retains the returned permalink, polls the
same POST with `permalinkKey`, stops immediately on terminal Error, and calls
GET only after Updated. A missing artifact after Updated is treated as cache
eviction and re-enters status polling with that same permalink; the old generic
GET-only retry loop is gone.
##########
superset/dashboards/api.py:
##########
@@ -1983,26 +1988,51 @@ def build_response(status_code: int) ->
WerkzeugResponse:
task_status=cache_payload.get_status(),
)
- if cache_payload.should_trigger_task(
- force, expected_scope=f"dashboard:{dashboard.id}"
+ if cached_payload is None or cache_payload.should_enqueue_task(
+ force, expected_scope=cache_scope
):
logger.info("Triggering screenshot ASYNC")
- cache_dashboard_screenshot.delay(
- username=get_current_user(),
- guest_token=(
- g.user.guest_token
- if get_current_user() and isinstance(g.user, GuestUser)
- else None
- ),
- dashboard_id=dashboard.id,
- dashboard_url=dashboard_url,
- thumb_size=thumb_size,
- window_size=window_size,
- cache_key=cache_key,
- force=force,
- )
+ cache_payload.pending()
+ cache_payload.set_scope(cache_scope)
+ if not screenshot_obj.store_cache_payload(cache_key,
cache_payload):
+ logger.error(
+ "Refusing to enqueue dashboard screenshot because Pending "
+ "state could not be cached: %s",
+ cache_key,
+ )
+ return self.response(
+ 503,
+ message=gettext("Screenshot cache is unavailable"),
+ )
+ try:
+ cache_dashboard_screenshot.delay(
+ username=get_current_user(),
+ guest_token=(
+ g.user.guest_token
+ if get_current_user() and isinstance(g.user, GuestUser)
+ else None
+ ),
+ dashboard_id=dashboard.id,
+ dashboard_url=dashboard_url,
+ thumb_size=thumb_size,
+ window_size=window_size,
+ cache_key=cache_key,
+ # The API has already invalidated the prior artifact by
+ # persisting PENDING. Avoid making duplicate queued tasks
+ # recompute after another worker has completed the request.
+ force=False,
+ )
+ except Exception: # pylint: disable=broad-except
+ cache_payload.error()
+ if not screenshot_obj.store_cache_payload(cache_key,
cache_payload):
+ logger.error(
+ "Could not persist dashboard screenshot Error state "
+ "after enqueue failure: %s",
+ cache_key,
+ )
+ raise
return build_response(202)
- return build_response(200)
+ return build_response(202 if cache_payload.is_in_progress() else 200)
Review Comment:
Addressed in both the endpoint docstring and generated OpenAPI description:
non-force requests keep returning the cached Error until
`THUMBNAIL_ERROR_CACHE_TTL` expires, and clients must send `force=true` to
retry sooner. The behavior is also covered by sticky-error and forced-retry
tests.
##########
superset/utils/screenshot_utils.py:
##########
@@ -277,6 +277,7 @@ class PlaywrightTimeout(PlaywrightError): # type:
ignore[no-redef] # noqa: N81
ALERT_SELECTOR = r'[role="alert"]'
EMPTY_SELECTOR = r".ant-empty, .ag-overlay-no-rows-wrapper:not(.ag-hidden)"
MISSING_CHART_SELECTOR = r".missing-chart-container"
+CHART_RENDERED_SELECTOR = r'.chart-container[data-chart-status="rendered"]'
Review Comment:
Addressed. The generic renderer marker is tied to the current render
generation and successful committed props: a same-viz refresh becomes ready
after the refreshed renderer commits, a viz change waits for the replacement
renderer, and stale callbacks cannot complete a newer generation. Tests cover
both same-viz refresh and viz-type replacement.
##########
superset/tasks/thumbnails.py:
##########
@@ -130,32 +132,51 @@ def cache_dashboard_screenshot( # pylint:
disable=too-many-arguments
# pylint: disable=import-outside-toplevel
from superset.models.dashboard import Dashboard
- if not thumbnail_cache:
+ if not is_cache_configured(thumbnail_cache):
logging.warning("No cache set, refusing to compute")
return
- dashboard = Dashboard.get(dashboard_id)
-
- logger.info("Caching dashboard: %s", dashboard_url)
-
- # Requests from Embedded should always use the Guest user
- if guest_token:
- current_user = security_manager.get_guest_user_from_token(guest_token)
- else:
- _, exec_username = get_executor(
- executors=current_app.config["THUMBNAIL_EXECUTORS"],
- model=dashboard,
- current_user=username,
- )
- current_user = security_manager.find_user(exec_username)
-
- with override_user(current_user):
- screenshot = DashboardScreenshot(dashboard_url, dashboard.digest)
- screenshot.cache_scope = f"dashboard:{dashboard.id}"
- screenshot.compute_and_cache(
- user=current_user,
- window_size=window_size,
- thumb_size=thumb_size,
- cache_key=cache_key,
- force=force,
+ try:
+ dashboard = Dashboard.get(dashboard_id)
+
+ logger.info("Caching dashboard: %s", dashboard_url)
+
+ # Requests from Embedded should always use the Guest user
+ if guest_token:
+ current_user =
security_manager.get_guest_user_from_token(guest_token)
+ else:
+ _, exec_username = get_executor(
+ executors=current_app.config["THUMBNAIL_EXECUTORS"],
+ model=dashboard,
+ current_user=username,
+ )
+ current_user = security_manager.find_user(exec_username)
+
+ with override_user(current_user):
+ screenshot = DashboardScreenshot(
+ dashboard_url,
+ dashboard.digest,
+ require_complete_capture=True,
+ )
+ screenshot.cache_scope = f"dashboard:{dashboard.id}"
+ screenshot.compute_and_cache(
+ user=current_user,
+ window_size=window_size,
+ thumb_size=thumb_size,
+ cache_key=cache_key,
+ force=force,
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.exception(
+ "Dashboard screenshot task failed before reaching a terminal
state: %s",
+ cache_key,
)
+ if cache_key:
+ error_payload =
ScreenshotCachePayload(scope=f"dashboard:{dashboard_id}")
Review Comment:
Addressed with producer and worker locks. The setup-failure fallback may
persist Error only after acquiring the worker lock; a duplicate that cannot
acquire it cannot overwrite another workers Computing state, and an already
completed payload is preserved. Targeted tests cover the active-worker and
completed-payload cases.
##########
superset/utils/cache.py:
##########
@@ -39,6 +39,56 @@
logger = logging.getLogger(__name__)
+def is_cache_configured(cache_instance: Cache | None) -> bool:
+ """Return whether a Flask-Caching instance has a configured backend."""
+
+ if cache_instance is None:
+ return False
+ try:
+ return not isinstance(cache_instance.cache, NullCache)
+ except AttributeError:
Review Comment:
Addressed. The production helper now inspects the real Flask-Caching
`.cache` backend without an AttributeError test-double fallback, and the
affected test doubles expose `.cache` explicitly. Cache configuration behavior
remains covered by unit tests.
--
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]
