geido commented on code in PR #44144:
URL: https://github.com/apache/superset/pull/44144#discussion_r3990322980
##########
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:
Fixed in e8bf94c8df. The API now advertises task_timeout_seconds from
THUMBNAIL_COMPUTING_CACHE_TTL, and the UI replaces the 30-poll cap with a
wall-clock watchdog aligned to that server budget while retaining a default
guard for a hung initial request. Tests cover completion after 96 seconds, a
420-second advertised lease, hung trigger/poll/download, and concurrent
operations; the Docker API advertised its configured 360 seconds.
##########
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:
Fixed in e8bf94c8df. OpenLayers now tracks load success and failure per
Source instead of latching one global error: a 404 is tolerated when that same
source has loaded coverage, a success from another source cannot mask an
all-failed source, and layer creation failures are explicit errors. Tests also
verify recovery and listener cleanup; Docker E2E passed partial coverage and
rejected all-failed/auth cases.
##########
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:
Fixed in e8bf94c8df. Active registry rejections now flow through
onRenderError and mark the chart container as error; stale rejections after a
viz change/unmount are ignored. Unit tests cover both paths, and Docker E2E
removed a real lazy-loaded chart chunk and observed Pending -> Computing ->
Error in 3.37s with no cached artifact.
##########
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:
Fixed in e8bf94c8df. DeckGL now uses generation-scoped per-source
success/failure state: a resource error invalidates the prior idle,
transient/partial tile failures recover after a successful tile plus fresh
idle, while auth/source-level failures and wholly failed sources report error.
Unit coverage includes transient, partial, all-failed, auth, stale-generation,
and generic-resource cases; Docker E2E verified mixed 200/500, initial 429s
then 200, all 500, and 401.
##########
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:
Fixed in e8bf94c8df. The stability timeout now includes the full configured
load-wait retry window plus the dwell/poll margin, bounded by the overall task
budget. Both standard and tiled capture also re-check strict readiness after
pixels are captured, discard a candidate caught during a render transition, and
retry up to three times. Unit tests cover long readiness recovery and repeated
post-capture drops; forced multi-tile Docker E2E passed.
##########
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:
Fixed in e8bf94c8df using the same shared map-resource state helper as
DeckGL. Point Cluster requires a fresh idle after an error, accepts useful
partial coverage only when that same source has a successful tile, and reports
auth/source-level or wholly failed sources as error. The same adversarial unit
and Docker tile-server matrix passed.
##########
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:
Fixed in e8bf94c8df. The lock-protected operation result is retained through
__exit__: a release failure after a successful enqueue returns the accepted
Pending payload, while an acquire failure remains a coordination error and an
enqueue failure remains the original error even if release also fails. Three
focused tests cover those branches; the final 8-way Docker concurrency probe
still produced exactly one Celery job.
--
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]
