sadpandajoe commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r3878463337
##########
superset/utils/screenshots.py:
##########
@@ -183,21 +191,66 @@ def get_invalid_image_reason(self) -> str | None:
return None
return validate_screenshot_image(self._image)
+ def _age_seconds(self) -> float | None:
+ """Seconds since this entry's timestamp, or None if the stored
+ timestamp is unusable -- a corrupt string (ValueError) or a legacy
+ tz-aware value that cannot be subtracted from naive now() (TypeError).
+ Callers treat None as 'past any TTL' so the entry self-heals."""
+ try:
+ return (
+ datetime.now() - datetime.fromisoformat(self.get_timestamp())
+ ).total_seconds()
+ except (ValueError, TypeError):
+ logger.warning(
+ "Unusable screenshot cache timestamp %r; "
+ "treating entry as expired/stale",
+ self.get_timestamp(),
+ )
+ return None
+
def is_error_cache_ttl_expired(self) -> bool:
- error_cache_ttl = app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+ # strict '>' (an entry exactly at the TTL is still fresh). An unusable
+ # timestamp (age is None) is treated as expired so the entry
self-heals.
+ age_seconds = self._age_seconds()
return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() > error_cache_ttl
+ age_seconds is None or age_seconds >
app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+ )
def is_computing_stale(self) -> bool:
"""Check if a COMPUTING status is stale (task likely failed or
stuck)."""
- computing_ttl = app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
+ # '>=' (unlike the strict '>' of the ERROR/UPDATED helpers). An
unusable
+ # timestamp (age is None) is treated as stale so the entry self-heals.
+ age_seconds = self._age_seconds()
return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() >= computing_ttl
+ age_seconds is None
+ or age_seconds >= app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
+ )
+
+ def is_updated_stale(self) -> bool:
+ """Whether a successfully-rendered (UPDATED) entry is old enough to be
+ recomputed. Returns False when the TTL is unset/0 (no-op unless an
operator
+ opts in). A timestamp we cannot use -- a corrupt string (ValueError)
or a
+ legacy tz-aware string that parses but cannot be subtracted from naive
+ now() (TypeError) -- is logged and treated as stale so it self-heals
rather
+ than being served forever."""
+ # `.get` (not `[]` like the sibling ERROR/COMPUTING helpers) on
purpose:
+ # a deployment whose config predates this key should silently disable
the
+ # feature, not raise KeyError. Checked first so a disabled feature
never
+ # parses/logs an unusable timestamp.
+ updated_ttl = app.config.get("THUMBNAIL_UPDATED_CACHE_TTL")
+ if not updated_ttl: # None or 0 => disabled
+ return False
+ # strict '>' (an image exactly at the TTL is still fresh), matching
+ # is_error_cache_ttl_expired -- not the '>=' of is_computing_stale. An
+ # unusable timestamp (age is None) is treated as stale so it
self-heals.
+ age_seconds = self._age_seconds()
+ return age_seconds is None or age_seconds > updated_ttl
Review Comment:
A future timestamp makes `age_seconds` negative, so this entry is served for
the TTL plus however far its clock is ahead rather than self-healing. Could
negative ages be treated as stale and covered by a future-timestamp test?
--
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]
