-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Changes from all commits
1214e7c
8044865
507d579
f0f9065
fdacbaf
b953fd4
bcf9849
4101407
d1a0ad1
e5d3a11
9a92f9a
d9c8639
dd77329
9c5f209
d5f8d42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -14,9 +14,15 @@ | |||||
# KIND, either express or implied. See the License for the | ||||||
# specific language governing permissions and limitations | ||||||
# under the License. | ||||||
import logging | ||||||
import math | ||||||
|
||||||
from flask import Flask | ||||||
from flask_babel import gettext as _ | ||||||
from flask_caching import Cache | ||||||
|
||||||
logger = logging.getLogger(__name__) | ||||||
|
||||||
|
||||||
class CacheManager: | ||||||
def __init__(self) -> None: | ||||||
|
@@ -28,41 +34,47 @@ def __init__(self) -> None: | |||||
self._filter_state_cache = Cache() | ||||||
self._explore_form_data_cache = Cache() | ||||||
|
||||||
@staticmethod | ||||||
def _init_cache( | ||||||
app: Flask, cache: Cache, cache_config_key: str, required: bool = False | ||||||
) -> None: | ||||||
cache_config = app.config[cache_config_key] | ||||||
cache_type = cache_config.get("CACHE_TYPE") | ||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm surprised this is not catched by There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
) | ||||||
|
||||||
if "CACHE_DEFAULT_TIMEOUT" not in cache_config: | ||||||
default_timeout = app.config.get("CACHE_DEFAULT_TIMEOUT") | ||||||
cache_config["CACHE_DEFAULT_TIMEOUT"] = default_timeout | ||||||
|
||||||
if required and cache_type in ("null", "NullCache"): | ||||||
raise Exception( | ||||||
_( | ||||||
"The CACHE_TYPE `%(cache_type)s` for `%(cache_config_key)s` is not " | ||||||
"supported. It is recommended to use `RedisCache`, " | ||||||
"`MemcachedCache` or another dedicated caching backend for " | ||||||
"production deployments", | ||||||
cache_type=cache_config["CACHE_TYPE"], | ||||||
cache_config_key=cache_config_key, | ||||||
), | ||||||
) | ||||||
cache.init_app(app, cache_config) | ||||||
|
||||||
def init_app(self, app: Flask) -> None: | ||||||
self._cache.init_app( | ||||||
app, | ||||||
{ | ||||||
"CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], | ||||||
**app.config["CACHE_CONFIG"], | ||||||
}, | ||||||
) | ||||||
self._data_cache.init_app( | ||||||
app, | ||||||
{ | ||||||
"CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], | ||||||
**app.config["DATA_CACHE_CONFIG"], | ||||||
}, | ||||||
) | ||||||
self._thumbnail_cache.init_app( | ||||||
app, | ||||||
{ | ||||||
"CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], | ||||||
**app.config["THUMBNAIL_CACHE_CONFIG"], | ||||||
}, | ||||||
) | ||||||
self._filter_state_cache.init_app( | ||||||
app, | ||||||
{ | ||||||
"CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], | ||||||
**app.config["FILTER_STATE_CACHE_CONFIG"], | ||||||
}, | ||||||
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 | ||||||
Comment on lines
+67
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||||||
) | ||||||
self._explore_form_data_cache.init_app( | ||||||
self._init_cache( | ||||||
app, | ||||||
{ | ||||||
"CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], | ||||||
**app.config["EXPLORE_FORM_DATA_CACHE_CONFIG"], | ||||||
}, | ||||||
self._explore_form_data_cache, | ||||||
"EXPLORE_FORM_DATA_CACHE_CONFIG", | ||||||
required=True, | ||||||
) | ||||||
|
||||||
@property | ||||||
|
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 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:
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.