QueryCache improvements for local caching #292
Conversation
…ady implemented by BaseContext anyway and makes accessing the cache much easier. Revised signature for QueryCache.remove(String) to be remove(QueryMetadata) to increase understandability. It was never clear how to use this method before. Added QueryCache.clearLocalCache method. This is now called automatically at the end of the request-response loop in StatelessContextRequestHandler and by BaseContext.finalize. This will prevent memory leaking from locally cached data in cases where the cache is not configured to expire entries based on time. Added QueryCache.debugListCacheKeys method to list all keys (prefixed by cache group) in the cache for debugging purposes.
|
It looks like the build failed just because it stalled on the last case. Is there a way to re-run it without adding another commit? |
|
Hi @johnthuss, I prefer not to bother with random glitches in travis (or docker) as long as it only a single failure. |
| if (key != null) { | ||
| for (String cache : cacheManager.getCacheNames()) { | ||
| getCache(cache).remove(key); | ||
| public void remove(QueryMetadata metadata) { |
There was a problem hiding this comment.
I believe we should keep old method for backward compatibility as it is part of public API (it may be marked as @Deprecated and deleted in a later release).
Also we try to mark all new public API methods with javadoc annotation @since CURRENT_VERSION.
There was a problem hiding this comment.
Thanks for your comments. I thought that would be the case.
| } | ||
|
|
||
| @Override | ||
| public List<String> debugListCacheKeys() { |
There was a problem hiding this comment.
Don't think this should in public API, we don't provide any tools to manage cache anyway. If someone need this, javax.cache.CacheManager can be injected and analyzed directly. Or, even better, specialized tools of actual JCache provider can be used.
There was a problem hiding this comment.
I'm not sure how injection could change this as the NestedQueryCache is hardcoded to wrap whatever cache is used. So to access the real cache directly you have to cast to NestedQueryCache and call getDelegate. But I guess that works, so I agree.
| } | ||
|
|
||
| @Override | ||
| public List<String> debugListCacheKeys() { |
There was a problem hiding this comment.
Don't think this should be part of public API, we don't provide any tools to manage cache (and we probably shouldn't). If someone need this, javax.cache.CacheManager can be injected and analyzed directly. Or, even better, specialized tools of actual JCache provider can be used.
| @@ -67,6 +69,10 @@ public void requestStart(ServletRequest request, ServletResponse response) { | |||
|
|
|||
| public void requestEnd(ServletRequest request, ServletResponse response) { | |||
| CayenneRuntime.bindThreadInjector(null); | |||
| ObjectContext context = BaseContext.getThreadObjectContext(); | |||
| if (context != null) { | |||
| ((BaseContext)context).getQueryCache().clearLocalCache(Optional.empty()); | |||
There was a problem hiding this comment.
Don't like this Optional.empty() thing, it's a little bit obscure what is it doing and why it's always empty.
Maybe we can push it directly to NestedQueryCache and call it explicitly from contexts that actually use it, but then we'll need some sort of context.cleanup() call.
There was a problem hiding this comment.
I don't like it either. The QueryCache interface doesn't have any concept of namespacing like the NestedQueryCache does, so any change that needs to target just the local cache and not shared&local must by necessity introduce something that represents the namespace or local/shared distinction. So I'm open to ideas. I'm not sure that NestedQueryCache really needs to exists as a separate class at all. The namespacing could be combined into to JCacheQueryCache as long as the QueryCache interface methods all support distinguishing between shared/local cache strategies, which it mostly does with this. The exception is removeGroup, which targets both shared and local, but that is probably fine as long as it is documented. I think combining NestedQueryCache would be my next choice.
|
I made changes based on your feedback. I left the debugListCacheKeys method in JCacheQueryCache but removed it from the QueryCache interface. The only alternative would be to expose the CacheManager directly, which would work if you prefer that. I don't know how to remove the Optional.empty() thing as I described above. The JCacheQueryCache.clearLocalCache method needs to know the namespace value from NestedQueryCache which it has no access to. |
|
Any more comments on this? I'd like to keep moving it forward. |
|
I'm going to close this and push something with a different approach. The LOCAL query cache should be able to be separate from the SHARED query cache in order to have an independent lifecycle (i.e. one that matches the ObjectContext it is connected to). |
|
These commits need some updates now, but it is a start in the direction we discussed on the dev mailing list. There are a few independent changes here, it's not all just about handling memory/invalidation, see my first comment for more info. |
Added getQueryCache to the ObjectContext interface since this is already implemented by BaseContext anyway and makes accessing the cache much easier.
Revised signature for QueryCache.remove(String) to be remove(QueryMetadata) to increase understandability. It was never clear how to use this method before.
Added QueryCache.clearLocalCache method. This is now called automatically at the end of the request-response loop in StatelessContextRequestHandler and by BaseContext.finalize. This will prevent memory leaking from locally cached data in cases where the cache is not configured to expire entries based on time.
Added QueryCache.debugListCacheKeys method to list all keys (prefixed by cache group) in the cache for debugging purposes.