[WIP] Proposal: Handle futureCache.get exceptions #65
Conversation
| return CompletableFuture.completedFuture(Optional.empty()); | ||
| } | ||
|
|
||
| return futureCache.get(key).handleAsync((v, exception) -> { |
There was a problem hiding this comment.
handleAsync use the standard ForkJoin pool - we make it a policy to never introduce threading into the library. Only the consuming code of this lib can really know what threading strategy to use.
If we accepted this change, this would have to be the version that takes and executor and it would have to come from DataLoaderOptions say
| if (futureCache.containsKey(cacheKey)) { | ||
| stats.incrementCacheHitCount(); | ||
| return futureCache.get(cacheKey); | ||
| return loadFromCache(key).thenApplyAsync((cachedValue) -> { |
There was a problem hiding this comment.
Same problem : thenApplyAsync uses the standard FookJoinPoll - see above
| var sourceResult = loadFromSource(key, loadContext); | ||
| sourceResult.thenAcceptAsync((sourceValue) -> { | ||
| if (cachingEnabled) { | ||
| futureCache.set(cacheKey, java.util.concurrent.CompletableFuture.supplyAsync(() -> sourceValue)); |
There was a problem hiding this comment.
There would be no need for
java.util.concurrent.CompletableFuture.supplyAsync(() -> sourceValue)
if you have a value then you can just do CompleteableFuture.completedValue(v)
That is a CF thats is finished successfully with value v
| return sourceResult.get(); | ||
| } catch (InterruptedException | java.util.concurrent.ExecutionException e) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This is a blocking call - sourceResult.get();
DataLoader MUST always be async - ALWAYS. We cant have this.
|
@bbakerman thanks for the feedback. I'll see if I can resolve these problems. |
I'd like to propose that the
DataLoaderHelperhandle situations wherecacheMap.getcompletes exceptionally. Currently this class assumes that if the cache contains a key, then the value associated with that key is the appropriate type.Note these changes have not been tested
Sorry if my use of
CompletableFutureis crude, I'd appreciate any composition advice.Motivation
I'm planning on using Redis as an expiring cache. The data has to be serialized and de-serialized on read and write. This cache is an external service and is shared between graphql server instances. Problems with network activity and differing application versions could cause issues with smooth serde.
Changes
The new flow for load is as follows.
a. Returns
Optional.emptyif: cache disabled, key not in cache, Cache.get fails or returns nullb. Returns
Optional<v>otherwiseloadedValuein cache