aminghadersohi commented on code in PR #38576:
URL: https://github.com/apache/superset/pull/38576#discussion_r3482424104
##########
superset/utils/screenshots.py:
##########
@@ -280,43 +281,66 @@ def compute_and_cache( # pylint:
disable=too-many-arguments
:return: Image payload
"""
cache_key = cache_key or self.get_cache_key(window_size, thumb_size)
- cache_payload = self.get_from_cache_key(cache_key) or
ScreenshotCachePayload()
- if not cache_payload.should_trigger_task(force=force):
- logger.info(
- "Skipping compute - already processed for thumbnail: %s",
cache_key
- )
- return
-
- window_size = window_size or self.window_size
- thumb_size = thumb_size or self.thumb_size
- logger.info("Processing url for thumbnail: %s", cache_key)
- cache_payload.computing()
- image = None
- # Assuming all sorts of things can go wrong with Selenium
try:
- logger.info("trying to generate screenshot")
- with
event_logger.log_context(f"screenshot.compute.{self.thumbnail_type}"):
- image = self.get_screenshot(user=user, window_size=window_size)
- except Exception as ex: # pylint: disable=broad-except
- logger.warning("Failed at generating thumbnail %s", ex,
exc_info=True)
- cache_payload.error()
- if image and window_size != thumb_size:
- try:
- image = self.resize_image(image, thumb_size=thumb_size)
- except Exception as ex: # pylint: disable=broad-except
- logger.warning("Failed at resizing thumbnail %s", ex,
exc_info=True)
- cache_payload.error()
+ with DistributedLock(
+ namespace="thumbnail",
+ key=cache_key,
+ ttl_seconds=app.config["THUMBNAIL_COMPUTING_CACHE_TTL"],
+ ):
+ cache_payload = (
+ self.get_from_cache_key(cache_key) or
ScreenshotCachePayload()
+ )
+ if not cache_payload.should_trigger_task(force=force):
+ logger.info(
+ "Skipping compute - already processed for thumbnail:
%s",
+ cache_key,
+ )
+ return
+
+ window_size = window_size or self.window_size
+ thumb_size = thumb_size or self.thumb_size
+ logger.info("Processing url for thumbnail: %s", cache_key)
+ cache_payload.computing()
+ self.cache.set(cache_key, cache_payload.to_dict())
image = None
-
- # Cache the result (success or error) to avoid immediate retries
- if image:
- with
event_logger.log_context(f"screenshot.cache.{self.thumbnail_type}"):
- cache_payload.update(image)
-
- logger.info("Caching thumbnail: %s", cache_key)
- self.cache.set(cache_key, cache_payload.to_dict())
- logger.info("Updated thumbnail cache; Status: %s",
cache_payload.get_status())
- return
+ # Assuming all sorts of things can go wrong with Selenium
+ try:
+ logger.info("trying to generate screenshot")
+ with event_logger.log_context(
+ f"screenshot.compute.{self.thumbnail_type}"
+ ):
+ image = self.get_screenshot(user=user,
window_size=window_size)
+ except Exception as ex: # pylint: disable=broad-except
+ logger.warning(
+ "Failed at generating thumbnail %s", ex, exc_info=True
+ )
+ cache_payload.error()
+ if image and window_size != thumb_size:
+ try:
+ image = self.resize_image(image, thumb_size=thumb_size)
+ except Exception as ex: # pylint: disable=broad-except
+ logger.warning(
+ "Failed at resizing thumbnail %s", ex,
exc_info=True
+ )
+ cache_payload.error()
+ image = None
+
+ if image:
+ with event_logger.log_context(
+ f"screenshot.cache.{self.thumbnail_type}"
+ ):
+ cache_payload.update(image)
+
+ logger.info("Caching thumbnail: %s", cache_key)
+ self.cache.set(cache_key, cache_payload.to_dict())
+ logger.info(
+ "Updated thumbnail cache; Status: %s",
cache_payload.get_status()
+ )
+ except AcquireDistributedLockFailedException:
+ logger.info(
+ "Skipping duplicate thumbnail task for %s - lock already held",
+ cache_key,
+ )
Review Comment:
Good catch — the finding is correct.
Added `LockAlreadyHeldException` as a subclass of
`AcquireDistributedLockFailedException`:
- `_acquire_redis`: raises `LockAlreadyHeldException` for the NX contention
case; keeps `AcquireDistributedLockFailedException` for `redis.RedisError`
(infrastructure failure)
- `_acquire_kv`: new `_kv_lock_on_error` raises `LockAlreadyHeldException`
on `IntegrityError` (unique-constraint = lock held) and
`AcquireDistributedLockFailedException` for other DB/encode errors
- `compute_and_cache`: now catches only `LockAlreadyHeldException` so
infrastructure failures propagate and Celery can retry
- Test updated to use `LockAlreadyHeldException` in the side_effect
Existing callers that catch the base `AcquireDistributedLockFailedException`
continue to work unchanged (subclass).
--
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]
