Skip to content

fix(thumbnails): handle remaining thumbnail states #33108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hxtmdev wants to merge 1 commit into from
Closed

fix(thumbnails): handle remaining thumbnail states #33108

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions superset/dashboards/api.py
View file
Open in desktop

@hxtmdev hxtmdev Apr 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untested but the screenshot code might have the same problem. I don't have time to test this as I don't use that functionality, should I create an issue?

Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@
DashboardScreenshot,
DEFAULT_DASHBOARD_WINDOW_SIZE,
ScreenshotCachePayload,
StatusValues,
)
from superset.utils.urls import get_url_path
from superset.views.base_api import (
Expand Down Expand Up @@ -1307,6 +1308,11 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse:
dashboard_id=dashboard.id,
force=False,
)

if (
cache_payload.should_trigger_task()

@mistercrunch mistercrunch Apr 14, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems this new criteria belongs in should_trigger_task (?) Or maybe I'm missing something?

@mistercrunch mistercrunch Apr 14, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like should_trigger_task is used in 4 places in the codebase, seems to be worth looking into those 3 other calls and seeing if logic should apply there, but from my perspective seems we should never trigger a task if it's in computing state.

@hxtmdev hxtmdev Apr 14, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO should_trigger_task is correct:
trigger task if either

  1. force
  2. PENDING
  3. ERROR and error ttl expired

COMPUTING should not trigger a new task because (as I understand it) that means it's already running.

So I add the COMPUTING check here because that's an additional case to return 202 (even if we didn't need to create a new task again).

@mistercrunch mistercrunch Apr 14, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COMPUTING should not trigger a new task because (as I understand it) that means it's already running.

the way I read your comment it seems we need to handle the COMPUTING case as part of should_trigger_task. Though maybe I'm missing something around force (?)

Let's dig a bit deeper.

Note that both charts and dashboards have essentially two contexts in which they call that. One is a "get me the image if it's there, and if you don't have it go and compute it, I'll expect a 202 if it doesn't exist yet", and another for "please compute the thumbnail (I'm not expecting an image here...), with a force flag". And I think we can assume that we call the second endpoint when the caller knows we need a new thumb or png (like we know that the the chart definition has changed, or we want a fresher thumb with new data for instance for a fresh report)

What I'd do or recommend:

  • remove force as a function argument, as it's context relevant for the caller, not for the function itself
  • have the callers call it in the context of if force or cache_payload.should_trigger_task():. It's the caller's job to decide if it wants to overrule should_trigger_task
  • handle COMPUTING in should_trigger_task assuming that if it's computing, we should NOT trigger a task. Here force can still overrule whatever should_trigger_task returns, which is what we want.

Maybe there are some small risks of race conditions (that also exist today AFAIK), around the rare case where say:

  • T1: force compute V1
  • T2: (while computing V1) start force computing V2
  • T3: somehow V2 finishes before V1
  • T4: V1 finishes computing after V2

So maybe ideally starting V2 in some cases should kill V1, but I wouldn't worry about that RN, but from experience it's hard to recall a celery job once dispatched...

@hxtmdev hxtmdev Apr 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, tbh I wasn't looking to kick off a bigger refactoring here. I was mainly just fixing the 500 that currently occurs when the screenshot is in the ERROR or COMPUTING state.

I only touched the COMPUTING part because the lines are adjacent and it's arguably part of the logic I was modifying anyway. Without changing that it will return 404 too, still better than the 500 it returns right now, but maybe not as correct.

If I reduce this PR to returning a clean 404 when the cache payload contains no image, would you be willing to merge that?

or cache_payload.status == StatusValues.COMPUTING
):
return self.response(
202,
cache_key=cache_key,
Expand All @@ -1316,12 +1322,15 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse:
task_status=cache_payload.get_status(),
)

self.incr_stats("from_cache", self.thumbnail.__name__)
return Response(
FileWrapper(cache_payload.get_image()),
mimetype="image/png",
direct_passthrough=True,
)
if image := cache_payload.get_image():
self.incr_stats("from_cache", self.thumbnail.__name__)
return Response(
FileWrapper(image),
mimetype="image/png",
direct_passthrough=True,
)
else:
return self.response_404()

@expose("/favorite_status/", methods=("GET",))
@protect()
Expand Down
Loading