-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(cache): default to SimpleCache in debug mode #18976
Conversation
5186888
to
bab168a
Compare
Codecov Report
@@ Coverage Diff @@
## master #18976 +/- ##
=======================================
Coverage 66.58% 66.58%
=======================================
Files 1641 1641
Lines 63533 63548 +15
Branches 6424 6424
=======================================
+ Hits 42303 42315 +12
- Misses 19551 19554 +3
Partials 1679 1679
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
bab168a
to
7b1926e
Compare
62cd84f
to
8b71ba1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. Thanks for the improvements. Left a comment.
a976056
to
c7b7c2e
Compare
c7b7c2e
to
05f0c80
Compare
05f0c80
to
9c5f209
Compare
61e9a11
to
501fb98
Compare
501fb98
to
d44a85d
Compare
d44a85d
to
d5f8d42
Compare
if app.debug and cache_type is None: | ||
cache_threshold = cache_config.get("CACHE_THRESHOLD", math.inf) | ||
cache_config.update( | ||
{"CACHE_TYPE": "SimpleCache", "CACHE_THRESHOLD": cache_threshold,} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{"CACHE_TYPE": "SimpleCache", "CACHE_THRESHOLD": cache_threshold,} | |
{"CACHE_TYPE": "SimpleCache", "CACHE_THRESHOLD": cache_threshold} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this is not catched by black
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this has been annoying me for a long time. I'm sure it's been fixed in a new version of black, would be nice to take a stab at updating it.
self._init_cache(app, self._cache, "CACHE_CONFIG") | ||
self._init_cache(app, self._data_cache, "DATA_CACHE_CONFIG") | ||
self._init_cache(app, self._thumbnail_cache, "THUMBNAIL_CACHE_CONFIG") | ||
self._init_cache( | ||
app, self._filter_state_cache, "FILTER_STATE_CACHE_CONFIG", required=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required but it would be a good idea to extract these strings into constants to avoid typing errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea - If I need to push changes to this PR I'll add that
@@ -26,6 +26,7 @@ assists people when migrating to a new version. | |||
|
|||
### Breaking Changes | |||
|
|||
- [18976](https://github.com/apache/superset/pull/18976): When running the app in debug mode, the app will default to use `SimpleCache` for `FILTER_STATE_CACHE_CONFIG` and `EXPLORE_FORM_DATA_CACHE_CONFIG`. When running in non-debug mode, a cache backend will need to be defined, otherwise the application will fail to start. For installations using Redis or other caching backends, it is recommended to use the same backend for both cache configs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For installations using Redis or other caching backends, it is recommended to use the same backend for both cache configs.
I'm wondering if there is a real need in configuring different cache expiration for both cache storages? If not, maybe we should just merge them---into something like SEMI_PERSISTENT_CACHE_CONFIG
(albeit it'd be a bigger breaking change)? I don't think it matters if we save explore state cache for 23 days more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ktmud I think the jury is still out on a few aspects of this feature:
- will people predominantly use direct URLs or permalinks for sharing
- what's the sweet spot timeout for these caches
- will we ever need multiple key-value states on a resource, or can we just collapse them into one key-value pair per resource
etc. I'm happy to keep iterating on this, and when we do a 3.0 release, I think it would be a perfect time to clean up some of the cruft we've accumulated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really liked how this PR progressed with the suggestions. It's clean and well documented. Thanks for all the hard work @villebro 👏🏼
* chore(cache): default to SimpleCache in debug mode * lint * clean up type * use util * fix integration test cache configs * remove util from cache manager * remove trailing comma * fix more tests * fix truthiness check * fix tests and improve deprecation notice * fix default cache threshold * move debug check to cache_manager * remove separate getter * update docs * remove default cache config (cherry picked from commit a04f1d4)
SUMMARY
Changes:
FILTER_STATE_CACHE_CONFIG
andEXPLORE_FORM_DATA_CACHE_CONFIG
toSimpleCache
when app is in debug mode.CACHE_TYPE
references to the deprecatednull
type withNullCache
and others (see: https://flask-caching.readthedocs.io/en/latest/#configuring-flask-caching)CacheConfig
type that had a remnant from [Caching] Ensure cache is always created #9020 where a callableCacheConfig
was supported, but was removed in chore: Using cache factory method #10887. TL;DR: now onlyDict[str, Any]
types are supported.NullCache
backend for required caches (Explore or Dashboard)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION