Value cache support #88
Conversation
value store to NoOp
| @@ -70,7 +72,7 @@ | |||
| * | |||
| * @return the cached value, or {@code null} if not found (depends on cache implementation) | |||
| */ | |||
| V get(U key); | |||
| CompletableFuture<V> get(K key); | |||
There was a problem hiding this comment.
The small brekaing change.
It was always a CompleteableFuture at runtime but now the signature reflects this more correctly
| } | ||
|
|
||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private CacheMap<Object, CompletableFuture<V>> determineCacheMap(DataLoaderOptions loaderOptions) { | ||
| return loaderOptions.cacheMap().isPresent() ? (CacheMap<Object, CompletableFuture<V>>) loaderOptions.cacheMap().get() : CacheMap.simpleMap(); | ||
| private CacheMap<Object, V> determineCacheMap(DataLoaderOptions loaderOptions) { |
There was a problem hiding this comment.
Should this be named determineFutureCache? I'm also not sure why we need to cast it and enforce a key type of object. Can't we use the K type erasure?
There was a problem hiding this comment.
the cache key that is looked up is actually and opaque object to it needs to be this.
The naming can be changed however
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private CachedValueStore<Object, V> determineCacheStore(DataLoaderOptions loaderOptions) { |
There was a problem hiding this comment.
Same observation and question here. Should this be called determineCachedValueStore and can we leave it as <K, V> instead of casting K to Object?
|
I would really appreciate your views on this |
| * @author <a href="https://github.com/craig-day">Craig Day</a> | ||
| */ | ||
| @PublicSpi | ||
| public interface CachedValueStore<K, V> { |
There was a problem hiding this comment.
I must say I am not really convinced of the name, but I also don't anything better out of my head :-)
| private final StatisticsCollector stats; | ||
| private final CacheMap<Object, V> futureCache; |
There was a problem hiding this comment.
so maybe FutureCache is a better name than CacheMap?
There was a problem hiding this comment.
Yes but that would definitely break everyone using it.
I want that better name but I am willing to wear the non breaking API change
There was a problem hiding this comment.
Some naming stuff, see comments.
But more importantly: this is a very specific implementation for a two level cache. Is it possible to find a more general abstraction and this two level cache is just one possible implementation? (I have not looked into it, so the answer maybe no)
This is a re-implementation of the PR that is #82
Both are trying to fix the issue outlined in Issue #45
It adds support for an external value cache as well as the previous local CompletableFuture cache.
It's basically the same code with some renames (I preferred the names) and also with extra documentation.
I also fixed a couple of edge case bugs that I found from more extensive testing
But it retains most of the code from the original PR and the idea of a 2 level cache.
This PR is technically a small breaking change. The signature of CacheMap has changed to reflect that it is in fact a cache of futures and not pure values (see #45). It is not anticipated that this will cause major problems but it is breaking and hence this needs to go out in a 3.x major version