sadpandajoe commented on code in PR #44662:
URL: https://github.com/apache/superset/pull/44662#discussion_r4343615252
##########
superset/config.py:
##########
@@ -1505,13 +1505,22 @@ def sync_theme_logo_href(
DATA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "NullCache"}
# Upper bound, in bytes, on the serialized size of a single value written to
the
-# data cache (chart and SQL query results). When a result's pickled size
exceeds
-# this threshold the value is NOT written to the cache: the chart still
renders,
-# but the next load re-queries the datasource instead of getting a cache hit.
This
-# protects the cache backend (e.g. Redis/Memcached) from being flooded by very
-# large result sets. Set to ``None`` to disable the check (the default).
Example:
-# 10 * 1024 * 1024 for a 10 MB limit.
-DATA_CACHE_MAX_VALUE_SIZE: int | None = None
+# data cache (chart and SQL query results, filter dropdown values, and
compatible
+# metrics/dimensions). A value whose pickled size exceeds this threshold is NOT
+# written to the cache, and any older value under the same key is removed so
it is
+# not served: the request still succeeds, and the value is recomputed the next
+# time it is requested, including the follow-up request a chart sends after a
+# background (async) query. This protects the cache backend (e.g.
+# Redis/Memcached) from being flooded by a heavy tail of very large result
sets,
+# which can drive the backend toward its memory limit and evict many smaller,
+# useful entries. Each skip emits a WARNING log naming the key and byte size
and
+# increments the ``skip_cache_value_too_large`` statsd counter. The default of
+# 10 MB comfortably exceeds typical chart/query payloads while excluding the
+# multi-tens-of-MB outliers responsible for cache pressure; raise it if
legitimate
+# results are being skipped, or set it to ``None`` to disable the check
entirely
+# (no serialization overhead is then incurred). Example: 20 * 1024 * 1024 for
+# 20 MB.
+DATA_CACHE_MAX_VALUE_SIZE: int | None = 10 * 1024 * 1024
Review Comment:
With `SEMANTIC_LAYER_CONTAINMENT_CACHE` enabled, this default turns on the
10 MB cap in `SemanticCacheService` (it reads `DATA_CACHE_MAX_VALUE_SIZE` as
`max_value_bytes`). In `execute()` an oversized result returns before
`repository.store(..., replace=force)`, so a forced refresh whose result has
grown past the cap leaves the older containment entry in place.
`skip_oversized_cache_value` only deletes the outer chart-cache key, so the
next ordinary load misses that key, is served the old containment result, and
caches the stale data again. Should the oversized/forced-refresh path in the
semantic cache also invalidate its existing entry, the way the chart cache now
does?
##########
UPDATING.md:
##########
@@ -660,6 +660,29 @@ Parent and child snapshots are rebuilt in the save
transaction after a captured
edit. If that rebuild fails, the save fails and must be rolled back, so an
incomplete snapshot is not exposed as restorable history.
+### `DATA_CACHE_MAX_VALUE_SIZE` now defaults to 10 MB
+
+`DATA_CACHE_MAX_VALUE_SIZE` limits the serialized size of a single value
written
+to the data cache. It defaults to `10 * 1024 * 1024` (10 MB) instead of `None`.
+Values larger than the limit are not stored in the cache, and any older cached
+copy stored under the same key is removed, so stale data is not shown. The
+request still returns its data, and the value is recomputed from the datasource
+the next time it is requested. This includes the follow-up request a chart
sends
+after a background (async) query finishes: that request recomputes the result
+instead of reading it from the cache, so an oversized async chart runs its
query
+twice. That recompute happens inside the web request itself, so a very slow,
very
+large query is subject to the web server's request timeout. For an async chart
+that shows contributions (percent of total), if the totals result is too large
to
+cache, the follow-up request computes the totals and the chart in the same way.
+
+The limit applies to every writer to the data cache: chart results, SQL query
Review Comment:
This says the limit applies to every writer to the data cache, but
`PrestoEngineSpec.get_function_names` and `latest_partition` still write
through `cache_manager.data_cache.memoize()`, which never goes through
`skip_oversized_cache_value`. With a 1 KB cap, a larger `SHOW FUNCTIONS` result
is still cached and no warning or `skip_cache_value_too_large` increment is
emitted; the semantic containment cache also uses its own Arrow-size check and
counter. Could the wording be narrowed to the writers actually covered (here
and in `cache.mdx`), or are those paths meant to be guarded too?
--
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]
