EnxDev commented on code in PR #44144:
URL: https://github.com/apache/superset/pull/44144#discussion_r4023644597
##########
superset/utils/screenshots.py:
##########
@@ -321,13 +407,69 @@ def get_from_cache_key(cls, cache_key: str) ->
ScreenshotCachePayload | None:
logger.info("Failed at getting from cache: %s", cache_key)
return None
- def compute_and_cache( # pylint: disable=too-many-arguments
+ @classmethod
+ def store_cache_payload(
+ cls,
+ cache_key: str,
+ cache_payload: ScreenshotCachePayload,
+ ) -> None:
+ """Persist screenshot state or raise when the backend rejects it."""
+
+ try:
+ stored = cls.cache.set(cache_key, cache_payload.to_dict())
+ except Exception as ex: # pylint: disable=broad-except
+ raise ScreenshotCacheError(
+ f"Could not persist screenshot cache key {cache_key}"
+ ) from ex
+ # Flask-Caching permits custom backends whose successful ``set``
+ # returns None, so only an explicit False is a failed write.
+ if stored is False:
+ raise ScreenshotCacheError(
+ f"Could not persist screenshot cache key {cache_key}"
+ )
+
+ @classmethod
+ def mark_cache_error_if_incomplete(cls, cache_key: str, scope: str) ->
None:
+ """Mark an accepted generation failed without clobbering another
worker."""
+
+ try:
+ with DistributedLock(
+ namespace="thumbnail",
+ key=cache_key,
+ ttl_seconds=app.config["THUMBNAIL_COMPUTING_CACHE_TTL"],
+ ):
+ cache_payload = cls.get_from_cache_key(cache_key)
Review Comment:
Could we use `raise_on_error=True` for this read too? A failed read doesn't
tell us whether this generation is incomplete. If a duplicate worker fails
during setup after another worker has completed, a transient GET timeout here
is swallowed as `None`; the fallback then replaces the existing `Updated` image
with `Error` and discards the artifact as soon as SET succeeds. Holding the
lock prevents concurrent writes, but doesn't protect the already-completed
result from this case.
I reproduced this with an existing Updated payload, a cache GET that raises
TimeoutError, and a successful SET: calling `mark_cache_error_if_incomplete`
leaves Error with no image. Propagating the read error to the existing `except
ScreenshotCacheError` would preserve the artifact. The initial read in strict
`compute_and_cache` has the same issue: it can treat a failed read of a
completed generation as permission to recompute. Could we opt both strict
worker reads into error propagation and add a failed-read/no-write regression?
--
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]
