Consolidate cache_memoize state and functionality into a CacheContext.
Review Request #12377 — Created June 15, 2022 and submitted
We pass a lot of state around the various functions that comprise
cache_memoize_iter(), and we have a lot of key
building that we duplicate everywhere. Some upcoming work will be making
a lot of this even more complex, so in preparation for that, this change
cleans up much of this state and logic.
There's now an internal
_CacheContextclass, which stores the state
being used for a caching operation. This includes:
- The cache connection
- The base key name passed to
- the full key name generated through
- The timeout
- Whether to compress large data
This simplifies a lot of function signatures.
It then provides functions for generating suitable keys (wrapping
make_cache_key(), subkeys (used for storing chunks), loading cached
data, and storing cached data. Right now, these are mostly very simple
wrappers, but they'll soon be extended for additional checks and
functionality, hence the consolidation of this logic.
There's also a performance improvement/race condition fix here. Both
cache_memoize_iter() previously checked if a
value was in cache, and then pulled it out. This requires two hits to
cache instead of one, and it's possible (though unlikely) that the value
could disappear in the meantime. In practice, we'd gracefully handle the
failure, but the two cache hits weren't necessary.
We now pull the value out, defaulting the result to a
sentinel if not found. We then make a determination based on this.
Aside from the performance improvement, there are no changes to how
the cache is utilized. This does make extensive improvements to our
documentation, and improves some logging (adding a missing "Cache miss"
cache_memoize() to go along with that in
and logging full cache keys).
All Djblets and Review Board unit tests pass.
Since this is purely internal API, do we really need to document changes?