Allow ValueCache to work with Publisher DataLoader #172
Conversation
We parameterise: - `DataLoaderValueCacheTest` - `ScheduledDataLoaderRegistryTest` to provide increased coverage across all DataLoader implementations. Notably, `DataLoaderValueCacheTest` is _not_ parameterised across the Publisher DataLoaders (yet) as these do not appear to work when a `ValueCache` is provided (which is what prompted this pull request). It will be fixed in a future pull request, at which point we will restore coverage.
We resolve two bugs in the interaction between Publisher `DataLoader`s and `ValueCache`s: - if the value was cached, we complete the corresponding queued futures immediately. - if the value was _not_ cached, we remember to provide the queued future to the invoker so that the future can eventually be completed.
| @@ -390,6 +390,9 @@ CompletableFuture<List<V>> invokeLoader(List<K> keys, List<Object> keyContexts, | |||
| missedKeyIndexes.add(i); | |||
| missedKeys.add(keys.get(i)); | |||
| missedKeyContexts.add(keyContexts.get(i)); | |||
| missedQueuedFutures.add(queuedFutures.get(i)); | |||
| } else { | |||
| queuedFutures.get(i).complete(cacheGet.get()); | |||
There was a problem hiding this comment.
why is the code queuedFutures.get(i).complete(cacheGet.get()); here???
Before it never used that parameter in this mutative way (eg completing a value if its not a cache get failure)
Is this the bug ??? Does it only happen on Publishers ???
There was a problem hiding this comment.
I cant find the test code for this fix
There was a problem hiding this comment.
Is this the bug ??? Does it only happen on Publishers ???
Yes - well, one of them (and it does indeed affect only the publishers). This particular line of code is needed if we have a cache hit in our value cache and want to return that - otherwise, we never complete, and our load returns a CompletableFuture that never completes. The way to check this is to comment out this line and re-run in particular DataLoaderValueCacheTest#batch_caching_works_as_expected.
I am not sure that this is the best way to solve this, but it does make the tests go green and doesn't appear to make anything else red - if this will cause other problems then we should add tests for that too (which I'm happy to do).
There was a problem hiding this comment.
I think the reason it did not work is here AFTER the invokeLoader method is called
private CompletableFuture<List<V>> dispatchQueueBatch(List<K> keys, List<Object> callContexts, List<CompletableFuture<V>> queuedFutures) {
stats.incrementBatchLoadCountBy(keys.size(), new IncrementBatchLoadCountByStatisticsContext<>(keys, callContexts));
CompletableFuture<List<V>> batchLoad = invokeLoader(keys, callContexts, queuedFutures, loaderOptions.cachingEnabled());
return batchLoad
.thenApply(values -> {
assertResultSize(keys, values);
if (isPublisher() || isMappedPublisher()) {
// We have already completed the queued futures by the time the overall batchLoad future has completed.
return values;
}
eg if its publisher its considered done while the others continue on to turn Trys into values. But I think where you have fixed it is approporiate
| DataLoaderOptions options = newOptions().setValueCache(customValueCache).setCachingEnabled(true).setBatchingEnabled(false); | ||
| DataLoader<String, String> identityLoader = idLoader(options, loadCalls); | ||
| DataLoader<String, String> identityLoader = factory.idLoader(options, loadCalls); | ||
|
|
||
| CompletableFuture<String> fA = identityLoader.load("a"); | ||
| CompletableFuture<String> fB = identityLoader.load("b"); |
There was a problem hiding this comment.
Where is the new test that shows the bug happening. I cant see it for all the other changes (and I have merged #171 )
There was a problem hiding this comment.
My bad for not explaining this. There aren't any "new" tests, as such - but we are now testing the existing tests here against the *PublisherDataLoader factories in addition to the standard ones (see 9a20334#r1901980139).
| @@ -33,7 +33,7 @@ | |||
| public class DataLoaderValueCacheTest { | |||
|
|
|||
| @ParameterizedTest | |||
| @MethodSource("org.dataloader.fixtures.parameterized.TestDataLoaderFactories#getWithoutPublisher") | |||
| @MethodSource("org.dataloader.fixtures.parameterized.TestDataLoaderFactories#get") | |||
There was a problem hiding this comment.
This is where we "add" the new tests - we are now testing this against PublisherDataLoaderFactory and MappedPublisherDataLoaderFactory.
There was a problem hiding this comment.
OK I get it now - we have tests for non publisher data loaders and publisher data loaders. So its working for the old and the new
We resolve two bugs in the interaction between Publisher
DataLoaders andValueCaches:This is intended to be merged following #171, as this pull request builds of this.