eschutho commented on code in PR #44662:
URL: https://github.com/apache/superset/pull/44662#discussion_r4211926745
##########
superset/config.py:
##########
@@ -1448,13 +1448,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:
Thanks, you're right. I reproduced it: with a totals result over the cap,
the totals task succeeds and publishes its key, then the contribution task
raises `Contribution totals not found in cache`, so the chart fails before the
browser sends its synchronous request.
Fixed in 4b7066acb8460a3f245e9669735804bfe6a4eec4:
-
[`_inject_contribution_totals`](https://github.com/apache/superset/blob/4b7066acb8460a3f245e9669735804bfe6a4eec4/superset/tasks/async_queries.py#L170-L201)
returns `False` instead of raising when the totals aren't in the cache.
- In that case
[`execute_chart_query`](https://github.com/apache/superset/blob/4b7066acb8460a3f245e9669735804bfe6a4eec4/superset/tasks/async_queries.py#L313-L326)
finishes without running or caching the contribution query and without
publishing a cache key. The task still succeeds, so the browser sends its usual
synchronous follow-up, and that request runs
[`ensure_totals_available`](https://github.com/apache/superset/blob/4b7066acb8460a3f245e9669735804bfe6a4eec4/superset/common/query_context_processor.py#L672-L673),
which recomputes the totals directly (no cache), then runs the contribution
query. So the chart renders, and no contribution result computed without its
totals is ever cached. A forced refresh behaves the same: since nothing was
cached, no "already refreshed" marker is written, and the follow-up recomputes.
I went this way rather than keeping the oversized totals in the cache,
because the contribution task only has its own query and can't re-run the
totals query itself. The trade-off is that for these charts the totals query
runs twice and the contribution query runs once, in the follow-up request. The
same path also covers a totals entry evicted before the contribution task reads
it, which used to fail too.
For the task boundary,
[`test_contribution_task_after_totals_task`](https://github.com/apache/superset/blob/4b7066acb8460a3f245e9669735804bfe6a4eec4/tests/unit_tests/common/test_oversized_result_readback.py#L237)
runs the real `execute_chart_query` body for the totals task and then the
dependent contribution task (only the worker plumbing is stubbed), against a
real in-memory cache with the cap applied:
- `fits`: the contribution task reads the cached totals, runs, and publishes
its key.
- `oversized`: the contribution task doesn't raise, runs nothing, caches
nothing and publishes nothing, and the follow-up request misses the cache and
computes the query. Before the fix this case failed with the exception above.
--
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]
