#13795 closed Uncategorized (fixed)
Add CACHE_KEY_PREFIX setting that prefixes the low-level cache API
Reported by: | Byron Ruth | Owned by: | Byron Ruth |
---|---|---|---|
Component: | Core (Cache system) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | inactivist@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I started the thread on django-developers http://groups.google.com/group/django-developers/browse_thread/thread/c6bfcaca1be25025.
Attachments (8)
Change History (24)
by , 14 years ago
Attachment: | cache_key_prefix_13357.diff added |
---|
comment:1 by , 14 years ago
Summary: | Add optional CACHE_KEY PREFIX setting that prefixes the low-level cache API → Add CACHE_KEY_PREFIX setting that prefixes the low-level cache API |
---|
comment:2 by , 14 years ago
Status: | new → assigned |
---|
follow-up: 5 comment:3 by , 14 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:5 by , 14 years ago
Replying to russellm:
- I think the _key() utility method should actually be public API, not a private method. Although a cache prefix is the obvious use case, I'm sure there are other potential uses -- modifying the cache key based on language settings is one possibility that springs to mind. If we're going to add a function for computing the cache key, it makes sense that it should be exposed and documented.
Now named make_key()
. Added a docstring noting that this can be overridden in subclassing for custom key making behavior.
- The key_prefix argument in cache backend constructors needs to have a default value (of ). Any existing subclasses of a cache class won't be passing down the key_prefix argument, so the default value needs to be provided down.
Fixed.
- When instantiating the default cache instance, you should use the keyword argument by name (i.e., key_prefix=settings.CACHE_KEY_PREFIX). This is just protection against backwards compatibility, plus a little bit of readability.
Updated.
- The tests are very thorough, but they miss one important test case -- that the prefix is being used at all. The tests could be simplified quite a bit, because there really no need to check the empty cache prefix case -- that's a degenerate case of the non-empty cache prefix case. However, it is important to check that when a cache prefix is in use, that the same key *without* the prefix isn't a cache hit (unless the cache key is empty). Much of the test duplication could be removed by making the non-empty cache prefix the default testing case, adding an extra test to ensure that the empty prefix case isn't a cache hit, and then adding a separate test case that runs exactly one test for the empty prefix case.
I simplified things a bit. Rather than subclassing for each backend, I merely added another test in BaseCacheTests to test for key clashes when using the prefix vs. not since that is of greatest concern.
- In the documentation for CACHE_MIDDLEWARE_PREFIX you should make mention of the fact that the CACHE_MIDDLEWARE_PREFIX will be appended to the CACHE_KEY_PREFIX - i.e., that CACHE_KEY_PREFIX is a global prefix.
Add a note to the CACHE_KEY_PREFIX section regarding CACHE_MIDDLEWARE_KEY_PREFIX.
by , 14 years ago
Attachment: | cache_key_prefix_13448.diff added |
---|
by , 14 years ago
Attachment: | cache_key_prefix_13959.diff added |
---|
by , 14 years ago
Attachment: | cache_key_prefix_13891.diff added |
---|
comment:7 by , 14 years ago
milestone: | → 1.3 |
---|---|
Patch needs improvement: | unset |
My last patch had a few problems with file based caching. The latest is up to date with trunk and passes all tests.
comment:8 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Unless someone is working on a better patch or sees a problem with mine, I'd say this one is ready for checkin.
comment:9 by , 14 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
I have an addition to the patch based on a suggestion from Jacob regarding cache versioning. I wrote a post yesterday on google groups, but it seems like it never got posted. I will reiterate here. Based on Eric Flo's http://github.com/ericflo/django-newcache cache backend, I think adding a CACHE_VERSION works well as a base setting, but more flexibility at runtime can be added by allowing for passing in a version keyword arg when getting/setting cache values:
cache.set('key', 'value', version=2) cache.get('key', version=1) # None
The default version will be CACHE_VERSION, but being able to pass a version during runtime could allow for more flexible cache versioning such as syncing versions of cache between multiple objects like a page and its components and cache invalidation, e.g. if the page's version is 2, and its components' version is 1, rebuild and cache.
Thoughts?
comment:10 by , 14 years ago
I should clarify that I have not written the patch just yet. I wanted to get some quick feedback if people would find it useful, then I will write it.
by , 14 years ago
Attachment: | cache_key_prefix_14190.diff added |
---|
comment:11 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I implemented cache versioning and added the appropriate unit tests. Please provide feedback.
follow-up: 13 comment:12 by , 14 years ago
I don't like the fact that validate_key() was moved to make_key(). If you override make_key() it makes it too easy to forget to validate the key before continuing. The rest of the patch looks fine.
comment:13 by , 14 years ago
Replying to agabel:
I don't like the fact that validate_key() was moved to make_key(). If you override make_key() it makes it too easy to forget to validate the key before continuing. The rest of the patch looks fine.
I agree except that validate_key() is being called within the BaseCache.make_key method, not in the make_key function outside of the class. Because that is confusing, I will make the BaseCache.make_key method *private* since the method is really a proxy for the make_key function.
by , 14 years ago
Attachment: | cache_key_prefix_14233.diff added |
---|
by , 14 years ago
Attachment: | cache_key_prefix_14405.diff added |
---|
comment:14 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:16 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
Severity: | → Normal |
Type: | → Uncategorized |
UI/UX: | unset |
This looks pretty good. A couple of quick points of feedback: