msyavuz commented on code in PR #44144:
URL: https://github.com/apache/superset/pull/44144#discussion_r3989183622
##########
superset-frontend/plugins/plugin-chart-point-cluster-map/src/MapLibre.tsx:
##########
@@ -160,25 +160,107 @@ function MapLibre({
const offsetHorizontal = (width * 0.5) / 100;
const offsetVertical = (height * 0.5) / 100;
- const bbox =
- bounds && bounds[0] && bounds[1]
- ? [
- bounds[0][0] - offsetHorizontal,
- bounds[0][1] - offsetVertical,
- bounds[1][0] + offsetHorizontal,
- bounds[1][1] + offsetVertical,
- ]
- : [-180, -90, 180, 90];
+ const bbox = useMemo(
+ () =>
+ bounds && bounds[0] && bounds[1]
+ ? [
+ bounds[0][0] - offsetHorizontal,
+ bounds[0][1] - offsetVertical,
+ bounds[1][0] + offsetHorizontal,
+ bounds[1][1] + offsetVertical,
+ ]
+ : [-180, -90, 180, 90],
+ [bounds, offsetHorizontal, offsetVertical],
+ );
- const clusters = clusterer.getClusters(bbox, Math.round(viewport.zoom));
+ const clusters = useMemo(
+ () => clusterer.getClusters(bbox, Math.round(viewport.zoom)),
+ [bbox, clusterer, viewport.zoom],
+ );
const theme = useTheme();
- const resolvedMapStyle: ResolvedMapStyle =
- mapProvider === 'mapbox'
- ? mapStyle || DEFAULT_MAP_STYLE
- : resolveMapStyle(mapStyle, DEFAULT_MAP_STYLE);
+ const resolvedMapStyle: ResolvedMapStyle = useMemo(
+ () =>
+ mapProvider === 'mapbox'
+ ? mapStyle || DEFAULT_MAP_STYLE
+ : resolveMapStyle(mapStyle, DEFAULT_MAP_STYLE),
+ [mapProvider, mapStyle],
+ );
const mapboxApiKey = mapProvider === 'mapbox' ? getMapboxApiKey() : '';
+ // The top-level renderer callback only proves that this React module
+ // mounted. Track both the base-map idle event and the imperative canvas
+ // redraw for the same inputs before declaring its pixels capture-ready.
+ const currentMapSource = useMemo(
+ () => ({
+ hasMapboxApiKey: Boolean(mapboxApiKey),
+ mapProvider,
+ resolvedMapStyle,
+ }),
+ [mapProvider, mapboxApiKey, resolvedMapStyle],
+ );
+ const currentMapRender = useMemo(
+ () => ({
+ height,
+ source: currentMapSource,
+ viewport,
+ width,
+ }),
+ [currentMapSource, height, viewport, width],
+ );
+ const currentOverlayRender = useMemo(
+ () => ({
+ aggregatorName,
+ clusters,
+ globalOpacity,
+ hasCustomMetric,
+ height,
+ mapProvider,
+ pointRadius,
+ pointRadiusUnit,
+ renderWhileDragging,
+ rgb,
+ viewport,
+ width,
+ }),
+ [
+ aggregatorName,
+ clusters,
+ globalOpacity,
+ hasCustomMetric,
+ height,
+ mapProvider,
+ pointRadius,
+ pointRadiusUnit,
+ renderWhileDragging,
+ rgb,
+ viewport,
+ width,
+ ],
+ );
+ const [completedMapRender, setCompletedMapRender] = useState<object | null>(
+ null,
+ );
+ const [completedOverlayRender, setCompletedOverlayRender] = useState<
+ object | null
+ >(null);
+ const [failedMapRender, setFailedMapRender] = useState<object | null>(null);
+ const handleMapIdle = useCallback(
+ () => setCompletedMapRender(currentMapRender),
+ [currentMapRender],
+ );
+ const handleMapError = useCallback(() => {
+ setFailedMapRender(currentMapSource);
Review Comment:
Same as DeckGLContainer: one transient tile error and this source can never
reach `rendered`.
##########
superset-frontend/plugins/preset-chart-deckgl/src/DeckGLContainer.tsx:
##########
@@ -90,26 +101,92 @@ export const DeckGLContainer = memo(
}, [tick]);
useEffect(() => {
- if (!isEqual(props.viewport, prevViewport)) {
- setViewState(props.viewport);
- }
- }, [prevViewport, props.viewport]);
+ setViewState(current =>
+ isEqual(current, props.viewport) ? current : props.viewport,
+ );
+ }, [props.viewport]);
const onMove = useCallback((evt: { viewState: JsonObject }) => {
setViewState(evt.viewState as Viewport);
setLastUpdate(Date.now());
}, []);
- const layers = useCallback(() => {
- // Support for layer factory
- if (props.layers.some(l => typeof l === 'function')) {
- return props.layers.map(l =>
- typeof l === 'function' ? l() : l,
- ) as Layer[];
- }
-
- return props.layers as Layer[];
- }, [props.layers]);
+ const isMapbox = props.mapProvider === 'mapbox';
+ const canRenderMap = !isMapbox || Boolean(props.mapboxApiKey);
+ const mapStyle = useMemo<ResolvedMapStyle>(
+ () =>
+ isMapbox
+ ? props.mapStyle || DEFAULT_MAP_STYLE
+ : resolveMapStyle(props.mapStyle, DEFAULT_MAP_STYLE),
+ [isMapbox, props.mapStyle],
+ );
+ const currentMapSource = useMemo(
+ () => ({
+ hasMapboxApiKey: Boolean(props.mapboxApiKey),
+ mapProvider: props.mapProvider,
+ mapStyle,
+ }),
+ [mapStyle, props.mapProvider, props.mapboxApiKey],
+ );
+ const currentMapRender = useMemo(
+ () => ({
+ height: props.height,
+ source: currentMapSource,
+ viewState,
+ width: props.width,
+ }),
+ [currentMapSource, props.height, props.width, viewState],
+ );
+ const resolvedLayers = useMemo(
+ () =>
+ canRenderMap
+ ? (props.layers.map(layer =>
+ typeof layer === 'function' ? layer() : layer,
+ ) as Layer[])
+ : [],
+ [canRenderMap, props.layers],
+ );
+ const currentDeckRender = useMemo(
+ () => ({
+ ...currentMapRender,
+ layers: resolvedLayers,
+ }),
+ [currentMapRender, resolvedLayers],
+ );
+ const currentDeckSource = useMemo(
+ () => ({
+ layers: resolvedLayers,
+ mapSource: currentMapSource,
+ }),
+ [currentMapSource, resolvedLayers],
+ );
+ const onMapIdle = useCallback(
+ () => setCompletedMapRender(currentMapRender),
+ [currentMapRender],
+ );
+ // MapLibre can emit idle after a source or tile error. Remember errors for
+ // this source generation so a move or later idle event cannot turn missing
+ // basemap pixels into a successful capture.
+ const onMapError = useCallback(() => {
+ setFailedMapRender(currentMapSource);
Review Comment:
Any non-404 map `error` (glyph/style fetch in an air-gapped deploy, a 429
from the tile CDN) pins this source to `loading` until the style prop changes,
so every deck.gl complete-capture times out where it previously captured with a
blank basemap. Intended?
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx:
##########
@@ -184,16 +216,37 @@ export const OlChartMap = (props: OlChartMapProps) => {
// stay on top, though.
const createdLayersPromises = configs.map(createLayer);
const createdLayers = await Promise.allSettled(createdLayersPromises);
+ if (cancelled) {
+ return;
+ }
+ let everyLayerCreated = true;
createdLayers.forEach((createdLayer, idx) => {
if (createdLayer.status === 'fulfilled' && createdLayer.value) {
+ const source = createdLayer.value.getSource();
+ if (source) {
+ source.addEventListener('tileloaderror', onLayerLoadError);
Review Comment:
OpenLayers fires `tileloaderror` for plain 404s (OSM above z19, any XYZ
source outside its coverage), so one missing tile sets `layerLoadFailed` for
the whole generation and the map can never report `rendered` until
`layerConfigs` changes.
##########
superset/utils/screenshot_utils.py:
##########
@@ -1196,37 +1340,49 @@ def _raise_if_budget_exhausted() -> None:
total_chart_holders = 0
contentful_chart_holders = 0
- if (
- total_chart_holders == 0
- and report_execution_context
- and report_execution_context.expected_chart_count
- ):
+ if total_chart_holders == 0 and strict_capture and
expected_chart_count:
logger.warning(
"report_capture_no_chart_holders tile=%s/%s "
"expected_holders=%s holder_count_failed=%s%s",
i + 1,
num_tiles,
- report_execution_context.expected_chart_count,
+ expected_chart_count,
holder_count_failed,
context_suffix,
)
# Take screenshot with clipping to capture only this tile's content
tile_screenshot: bytes | None = None
for capture_attempt in range(1,
TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS + 1):
- if report_execution_context:
+ if strict_capture:
stable_wait = _timeout_seconds(
"capture_readiness_stability",
+ requested_seconds=(
+ None
+ if report_execution_context
+ else (REPORT_CAPTURE_READINESS_STABILITY_MS +
1000) / 1000
+ ),
reserve_seconds=(
report_execution_context.readiness_reserve_seconds
+ if report_execution_context
+ else 1.0
),
)
try:
waited_for_stability = wait_for_stable_readiness(
Review Comment:
If readiness drops for more than 1s inside this dwell on the API path (a
chart re-querying after a filter default lands), the `PlaywrightTimeout` below
raises straight through to a day-long sticky `Error`. Re-enter the readiness
wait instead of failing the capture?
##########
superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts:
##########
@@ -35,6 +35,13 @@ import { DownloadScreenshotFormat } from
'../components/menu/DownloadMenuItems/t
const RETRY_INTERVAL = 3000;
const MAX_RETRIES = 30;
Review Comment:
Still ~90s total (30 × 3s). `SCREENSHOT_LOAD_WAIT` alone is 60s and tiled
captures wait per tile, so a slow dashboard still lands on the same generic
toast. Is that enough now that the backend keeps the task alive, or should the
cap track the task budget?
##########
superset/utils/screenshots.py:
##########
@@ -321,13 +386,160 @@ def get_from_cache_key(cls, cache_key: str) ->
ScreenshotCachePayload | None:
logger.info("Failed at getting from cache: %s", cache_key)
return None
+ @classmethod
+ def store_cache_payload(
+ cls,
+ cache_key: str,
+ cache_payload: ScreenshotCachePayload,
+ ) -> bool:
+ """Persist screenshot state and report backend write failures."""
+
+ return set_cache_value(cls.cache, cache_key, cache_payload.to_dict())
+
+ @classmethod
+ def prepare_and_enqueue_task(
+ cls,
+ cache_key: str,
+ *,
+ force: bool,
+ scope: str,
+ enqueue: Callable[[], None],
+ ) -> tuple[ScreenshotCachePayload, bool]:
+ """Atomically claim a cache key and publish its API task.
+
+ Producers use a short, separate lock from workers. Holding it through
+ broker publication prevents a second producer from colliding with a
+ fast worker or racing enqueue-failure cleanup. A leaked producer lock
+ cannot block an already accepted worker.
+
+ :return: The latest payload and whether the caller owns the enqueue.
+ """
+
+ try:
+ with DistributedLock(
+ namespace="thumbnail_enqueue",
+ key=cache_key,
+ ):
+ cache_payload = cls.get_from_cache_key(cache_key)
+ cache_payload = cache_payload or ScreenshotCachePayload()
+ if not cache_payload.should_enqueue_task(
+ force,
+ expected_scope=scope,
+ ):
+ return cache_payload, False
+ cache_payload.pending()
+ cache_payload.set_scope(scope)
+ cls._store_cache_payload_or_raise(cache_key, cache_payload)
+ try:
+ enqueue()
+ except Exception: # pylint: disable=broad-except
+ try:
+ if not cls.store_error_if_no_active_task(cache_key,
scope):
+ logger.error(
+ "Could not persist screenshot Error state
after "
+ "enqueue failure: %s",
+ cache_key,
+ )
+ except ScreenshotCacheError:
+ logger.exception(
+ "Could not inspect screenshot state after enqueue "
+ "failure: %s",
+ cache_key,
+ )
+ raise
+ return cache_payload, True
+ except LockAlreadyHeldException:
+ # Another API producer owns publication for this key. Polling will
+ # observe its Pending transition or terminal result.
+ cache_payload = ScreenshotCachePayload(scope=scope)
+ cache_payload.pending()
+ return cache_payload, False
+ except (
+ AcquireDistributedLockFailedException,
+ ReleaseDistributedLockFailedException,
+ ) as ex:
+ raise ScreenshotCacheWriteError(
Review Comment:
A `ReleaseDistributedLockFailedException` after `enqueue()` succeeded turns
an accepted, `Pending`-persisted task into a 503 to the client. Catch release
failures separately and return the payload.
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/ChartWrapper.tsx:
##########
@@ -30,19 +30,36 @@ export const ChartWrapper: FC<ChartWrapperProps> = ({
width,
chartConfig,
locale,
+ onRenderComplete,
}) => {
const [Chart, setChart] = useState<any>();
- const getChartFromRegistry = async (vizType: string) => {
- const registry = getChartComponentRegistry();
- const c = await registry.getAsPromise(vizType);
- setChart(() => c);
- };
-
useEffect(() => {
- getChartFromRegistry(vizType);
+ let active = true;
+ setChart(undefined);
+ getChartComponentRegistry()
+ .getAsPromise(vizType)
+ .then(chart => {
+ if (active) {
+ setChart(() => chart);
+ }
+ })
+ .catch(error => {
+ if (active) {
+ console.warn(`Could not load cartodiagram chart: ${error}`);
Review Comment:
A rejected registry load only warns; `Chart` stays undefined,
`onRenderComplete` never fires, and every container stays
`data-superset-map-status="loading"` forever, so the whole capture times out
instead of failing.
--
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]
