msyavuz commented on code in PR #44144:
URL: https://github.com/apache/superset/pull/44144#discussion_r3981534068
##########
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:
`check-openapi-spec-drift` is red: this needs `superset update-api-docs`
rerun and the regenerated `docs/static/resources/openapi.json` committed.
##########
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:
`THUMBNAIL_ERROR_CACHE_TTL` defaults to one day, so a non-force re-POST
returns `Error` for a day; the docstring should say clients must send `force`
to retry.
##########
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:
`pending()` nulls the image, so `force` on a good artifact 404s `image_url`
until recompute finishes. `useDownloadScreenshot` always sends `force` and only
polls the GET (3s x 30), so a dashboard slower than `SCREENSHOT_LOAD_WAIT` now
ends in a generic max-retries failure after 90s; worth a follow-up to poll the
POST and stop on `Error`.
##########
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:
If this duplicate task failed before taking the lock, this overwrites
another worker's live `Computing` on the same key; benign since that worker
writes `Updated` at the end, but pollers can see `Error` flip to `Updated`.
##########
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:
`rendered` is set from the loadable renderer's mount effect, so a chart that
stays mounted across a re-query (`suppressLoadingSpinner`, e.g. dashboard
`refresh_frequency` in standalone) stays at `success` and reports
`plugin_loading` until timeout. Intentional?
##########
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:
Does maplibre/mapbox still emit `idle` when the style fails to load (Mapbox
401, no tile egress, no WebGL)? If not, the host stays unpainted and every
capture containing a map chart times out.
##########
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:
This body is shared with scheduled reports, so reports now also require
`rendered` plus deck.gl `idle` on every chart. Can the deck.gl gate be limited
to `require_complete_capture` (or split out) so a map whose `idle` never fires
in the worker doesn't turn a working report into a timeout?
##########
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:
Test accommodation in prod code; give the test doubles a `.cache` attribute
instead.
--
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]
