From 50e4179d4d0f619419d8dda49598362e3ea5eed5 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 28 Jun 2021 22:21:56 +1000 Subject: [PATCH 1/5] A re-work of the https://github.com/graphql-java/java-dataloader/pull/82 that renamed a few things, tested more and fixed a few bugs --- build.gradle | 1 + src/main/java/org/dataloader/CacheMap.java | 24 +- .../java/org/dataloader/CachedValueStore.java | 92 +++++++ src/main/java/org/dataloader/DataLoader.java | 47 +++- .../java/org/dataloader/DataLoaderHelper.java | 107 ++++++-- .../org/dataloader/DataLoaderOptions.java | 35 ++- .../org/dataloader/impl/DefaultCacheMap.java | 18 +- .../impl/DefaultCachedValueStore.java | 62 +++++ src/test/java/ReadmeExamples.java | 4 +- .../DataLoaderCachedValueStoreTest.java | 246 ++++++++++++++++++ .../java/org/dataloader/DataLoaderTest.java | 1 + .../fixtures/CaffeineCachedValueStore.java | 51 ++++ .../{ => fixtures}/CustomCacheMap.java | 11 +- .../fixtures/CustomCachedValueStore.java | 41 +++ 14 files changed, 679 insertions(+), 61 deletions(-) create mode 100644 src/main/java/org/dataloader/CachedValueStore.java create mode 100644 src/main/java/org/dataloader/impl/DefaultCachedValueStore.java create mode 100644 src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java create mode 100644 src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java rename src/test/java/org/dataloader/{ => fixtures}/CustomCacheMap.java (68%) create mode 100644 src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java diff --git a/build.gradle b/build.gradle index 86004bd..cdbca84 100644 --- a/build.gradle +++ b/build.gradle @@ -58,6 +58,7 @@ dependencies { testCompile 'org.slf4j:slf4j-simple:' + slf4jVersion testCompile "junit:junit:4.12" testCompile 'org.awaitility:awaitility:2.0.0' + testImplementation 'com.github.ben-manes.caffeine:caffeine:2.9.0' } task sourcesJar(type: Jar) { diff --git a/src/main/java/org/dataloader/CacheMap.java b/src/main/java/org/dataloader/CacheMap.java index 9373a77..9ae98f5 100644 --- a/src/main/java/org/dataloader/CacheMap.java +++ b/src/main/java/org/dataloader/CacheMap.java @@ -28,37 +28,39 @@ * implementation could also have used a regular {@link java.util.Map} instead of this {@link CacheMap}, but * this aligns better to the reference data loader implementation provided by Facebook *

- * Also it doesn't require you to implement the full set of map overloads, just the required methods. + * This is really a cache of completed {@link CompletableFuture} values in memory. It is used, when caching is enabled, to + * give back the same future to any code that may call it. if you need a cache of the underlying values that is possible external to the JVM + * then you will want to use {{@link CachedValueStore}} which is designed for external cache access. * - * @param type parameter indicating the type of the cache keys + * @param type parameter indicating the type of the cache keys * @param type parameter indicating the type of the data that is cached * * @author Arnold Schrijver * @author Brad Baker */ @PublicSpi -public interface CacheMap { +public interface CacheMap { /** * Creates a new cache map, using the default implementation that is based on a {@link java.util.LinkedHashMap}. * - * @param type parameter indicating the type of the cache keys + * @param type parameter indicating the type of the cache keys * @param type parameter indicating the type of the data that is cached * * @return the cache map */ - static CacheMap> simpleMap() { + static CacheMap simpleMap() { return new DefaultCacheMap<>(); } /** - * Checks whether the specified key is contained in the cach map. + * Checks whether the specified key is contained in the cache map. * * @param key the key to check * * @return {@code true} if the cache contains the key, {@code false} otherwise */ - boolean containsKey(U key); + boolean containsKey(K key); /** * Gets the specified key from the cache map. @@ -70,7 +72,7 @@ static CacheMap> simpleMap() { * * @return the cached value, or {@code null} if not found (depends on cache implementation) */ - V get(U key); + CompletableFuture get(K key); /** * Creates a new cache map entry with the specified key and value, or updates the value if the key already exists. @@ -80,7 +82,7 @@ static CacheMap> simpleMap() { * * @return the cache map for fluent coding */ - CacheMap set(U key, V value); + CacheMap set(K key, CompletableFuture value); /** * Deletes the entry with the specified key from the cache map, if it exists. @@ -89,12 +91,12 @@ static CacheMap> simpleMap() { * * @return the cache map for fluent coding */ - CacheMap delete(U key); + CacheMap delete(K key); /** * Clears all entries of the cache map * * @return the cache map for fluent coding */ - CacheMap clear(); + CacheMap clear(); } diff --git a/src/main/java/org/dataloader/CachedValueStore.java b/src/main/java/org/dataloader/CachedValueStore.java new file mode 100644 index 0000000..2135330 --- /dev/null +++ b/src/main/java/org/dataloader/CachedValueStore.java @@ -0,0 +1,92 @@ +package org.dataloader; + +import org.dataloader.annotations.PublicSpi; +import org.dataloader.impl.DefaultCachedValueStore; + +import java.util.List; +import java.util.concurrent.CompletableFuture; + +/** + * Cache value store for data loaders that use caching and want a long-lived or external cache. + *

+ * The default implementation is a no-op store which replies with the key always missing and doesn't + * store any actual results. This is to avoid duplicating the stored data between the {@link CacheMap} + * and the store. + *

+ * The API signature uses completable futures because the backing implementation MAY be a remote external cache + * and hence exceptions may happen in retrieving values. + * + * @param the type of cache keys + * @param the type of cache values + * + * @author Craig Day + */ +@PublicSpi +public interface CachedValueStore { + + /** + * Creates a new store, using the default no-op implementation. + * + * @param the type of cache keys + * @param the type of cache values + * + * @return the cache store + */ + static CachedValueStore defaultStore() { + return new DefaultCachedValueStore<>(); + } + + /** + * Checks whether the specified key is contained in the store. + *

+ * {@link DataLoader} first calls {@link #containsKey(Object)} and then calls {@link #get(Object)}. If the + * backing cache implementation cannot answer the `containsKey` call then simply return true and the + * following `get` call can complete exceptionally to cause the {@link DataLoader} + * to enqueue the key to the {@link BatchLoader#load(List)} call since it is not present in cache. + * + * @param key the key to check if its present in the cache + * + * @return {@code true} if the cache contains the key, {@code false} otherwise + */ + CompletableFuture containsKey(K key); + + /** + * Gets the specified key from the store. + * + * @param key the key to retrieve + * + * @return a future containing the cached value (which maybe null) or an exception if the key does + * not exist in the cache. + * + * IMPORTANT: The future may fail if the key does not exist depending on implementation. Implementations should + * return an exceptional future if the key is not present in the cache, not null which is a valid value + * for a key. + */ + CompletableFuture get(K key); + + /** + * Stores the value with the specified key, or updates it if the key already exists. + * + * @param key the key to store + * @param value the value to store + * + * @return a future containing the stored value for fluent composition + */ + CompletableFuture set(K key, V value); + + /** + * Deletes the entry with the specified key from the store, if it exists. + * + * @param key the key to delete + * + * @return a void future for error handling and fluent composition + */ + CompletableFuture delete(K key); + + /** + * Clears all entries from the store. + * + * @return a void future for error handling and fluent composition + */ + CompletableFuture clear(); +} \ No newline at end of file diff --git a/src/main/java/org/dataloader/DataLoader.java b/src/main/java/org/dataloader/DataLoader.java index 18e7900..8ed5b13 100644 --- a/src/main/java/org/dataloader/DataLoader.java +++ b/src/main/java/org/dataloader/DataLoader.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; import static org.dataloader.impl.Assertions.nonNull; @@ -64,8 +65,9 @@ public class DataLoader { private final DataLoaderHelper helper; - private final CacheMap> futureCache; private final StatisticsCollector stats; + private final CacheMap futureCache; + private final CachedValueStore cachedValueStore; /** * Creates new DataLoader with the specified batch loader function and default options @@ -414,18 +416,23 @@ public DataLoader(BatchLoader batchLoadFunction, DataLoaderOptions options DataLoader(Object batchLoadFunction, DataLoaderOptions options, Clock clock) { DataLoaderOptions loaderOptions = options == null ? new DataLoaderOptions() : options; this.futureCache = determineCacheMap(loaderOptions); + this.cachedValueStore = determineCacheStore(loaderOptions); // order of keys matter in data loader this.stats = nonNull(loaderOptions.getStatisticsCollector()); - this.helper = new DataLoaderHelper<>(this, batchLoadFunction, loaderOptions, this.futureCache, this.stats, clock); + this.helper = new DataLoaderHelper<>(this, batchLoadFunction, loaderOptions, this.futureCache, this.cachedValueStore, this.stats, clock); } @SuppressWarnings("unchecked") - private CacheMap> determineCacheMap(DataLoaderOptions loaderOptions) { - return loaderOptions.cacheMap().isPresent() ? (CacheMap>) loaderOptions.cacheMap().get() : CacheMap.simpleMap(); + private CacheMap determineCacheMap(DataLoaderOptions loaderOptions) { + return (CacheMap) loaderOptions.cacheMap().orElseGet(CacheMap::simpleMap); } + @SuppressWarnings("unchecked") + private CachedValueStore determineCacheStore(DataLoaderOptions loaderOptions) { + return (CachedValueStore) loaderOptions.cachedValueStore().orElseGet(CachedValueStore::defaultStore); + } /** * This returns the last instant the data loader was dispatched. When the data loader is created this value is set to now. @@ -628,9 +635,24 @@ public int dispatchDepth() { * @return the data loader for fluent coding */ public DataLoader clear(K key) { + return clear(key, (v, e) -> { + }); + } + + /** + * Clears the future with the specified key from the cache remote value store, if caching is enabled + * and a remote store is set, so it will be re-fetched and stored on the next load request. + * + * @param key the key to remove + * @param handler a handler that will be called after the async remote clear completes + * + * @return the data loader for fluent coding + */ + public DataLoader clear(K key, BiConsumer handler) { Object cacheKey = getCacheKey(key); synchronized (this) { futureCache.delete(cacheKey); + cachedValueStore.delete(key).whenComplete(handler); } return this; } @@ -641,14 +663,29 @@ public DataLoader clear(K key) { * @return the data loader for fluent coding */ public DataLoader clearAll() { + return clearAll((v, e) -> { + }); + } + + /** + * Clears the entire cache map of the loader, and of the cached value store. + * + * @param handler a handler that will be called after the async remote clear all completes + * + * @return the data loader for fluent coding + */ + public DataLoader clearAll(BiConsumer handler) { synchronized (this) { futureCache.clear(); + cachedValueStore.clear().whenComplete(handler); } return this; } /** - * Primes the cache with the given key and value. + * Primes the cache with the given key and value. Note this will only prime the future cache + * and not the value store. Use {@link CachedValueStore#set(Object, Object)} if you want + * o prime it with values before use * * @param key the key * @param value the value diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java index bd93814..af3417e 100644 --- a/src/main/java/org/dataloader/DataLoaderHelper.java +++ b/src/main/java/org/dataloader/DataLoaderHelper.java @@ -16,6 +16,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BiConsumer; import java.util.stream.Collectors; import static java.util.Collections.emptyList; @@ -61,17 +62,25 @@ Object getCallContext() { private final DataLoader dataLoader; private final Object batchLoadFunction; private final DataLoaderOptions loaderOptions; - private final CacheMap> futureCache; + private final CacheMap futureCache; + private final CachedValueStore cachedValueStore; private final List>> loaderQueue; private final StatisticsCollector stats; private final Clock clock; private final AtomicReference lastDispatchTime; - DataLoaderHelper(DataLoader dataLoader, Object batchLoadFunction, DataLoaderOptions loaderOptions, CacheMap> futureCache, StatisticsCollector stats, Clock clock) { + DataLoaderHelper(DataLoader dataLoader, + Object batchLoadFunction, + DataLoaderOptions loaderOptions, + CacheMap futureCache, + CachedValueStore cachedValueStore, + StatisticsCollector stats, + Clock clock) { this.dataLoader = dataLoader; this.batchLoadFunction = batchLoadFunction; this.loaderOptions = loaderOptions; this.futureCache = futureCache; + this.cachedValueStore = cachedValueStore; this.loaderQueue = new ArrayList<>(); this.stats = stats; this.clock = clock; @@ -120,35 +129,13 @@ CompletableFuture load(K key, Object loadContext) { boolean batchingEnabled = loaderOptions.batchingEnabled(); boolean cachingEnabled = loaderOptions.cachingEnabled(); - Object cacheKey = null; - if (cachingEnabled) { - if (loadContext == null) { - cacheKey = getCacheKey(key); - } else { - cacheKey = getCacheKeyWithContext(key, loadContext); - } - } stats.incrementLoadCount(); if (cachingEnabled) { - if (futureCache.containsKey(cacheKey)) { - stats.incrementCacheHitCount(); - return futureCache.get(cacheKey); - } - } - - CompletableFuture future = new CompletableFuture<>(); - if (batchingEnabled) { - loaderQueue.add(new LoaderQueueEntry<>(key, future, loadContext)); + return loadFromCache(key, loadContext, batchingEnabled); } else { - stats.incrementBatchLoadCountBy(1); - // immediate execution of batch function - future = invokeLoaderImmediately(key, loadContext); + return queueOrInvokeLoader(key, loadContext, batchingEnabled); } - if (cachingEnabled) { - futureCache.set(cacheKey, future); - } - return future; } } @@ -296,6 +283,74 @@ private void possiblyClearCacheEntriesOnExceptions(List keys) { } } + private CompletableFuture loadFromCache(K key, Object loadContext, boolean batchingEnabled) { + final Object cacheKey = loadContext == null ? getCacheKey(key) : getCacheKeyWithContext(key, loadContext); + + if (futureCache.containsKey(cacheKey)) { + // We already have a promise for this key, no need to check value cache or queue up load + stats.incrementCacheHitCount(); + return futureCache.get(cacheKey); + } + + /* + We haven't been asked for this key yet. We want to do one of two things: + + 1. Check if our cache store has it. If so: + a. Get the value from the cache store + b. Add a recovery case so we queue the load if fetching from cache store fails + c. Put that future in our futureCache to hit the early return next time + d. Return the resilient future + 2. If not in value cache: + a. queue or invoke the load + b. Add a success handler to store the result in the cache store + c. Return the result + */ + final CompletableFuture future = new CompletableFuture<>(); + + cachedValueStore.containsKey(cacheKey).whenComplete((hasKey, containsCallEx) -> { + boolean containsKey = containsCallEx == null && Boolean.TRUE.equals(hasKey); + if (containsKey) { + cachedValueStore.get(cacheKey).whenComplete((cachedValue, getCallEx) -> { + if (getCallEx == null) { + future.complete(cachedValue); + } else { + queueOrInvokeLoader(key, loadContext, batchingEnabled) + .whenComplete(setValueIntoCacheAndCompleteFuture(cacheKey, future)); + } + }); + } else { + queueOrInvokeLoader(key, loadContext, batchingEnabled) + .whenComplete(setValueIntoCacheAndCompleteFuture(cacheKey, future)); + } + }); + + futureCache.set(cacheKey, future); + + return future; + } + + private BiConsumer setValueIntoCacheAndCompleteFuture(Object cacheKey, CompletableFuture future) { + return (result, loadCallEx) -> { + if (loadCallEx == null) { + cachedValueStore.set(cacheKey, result) + .whenComplete((v, setCallExIgnored) -> future.complete(result)); + } else { + future.completeExceptionally(loadCallEx); + } + }; + } + + private CompletableFuture queueOrInvokeLoader(K key, Object loadContext, boolean batchingEnabled) { + if (batchingEnabled) { + CompletableFuture future = new CompletableFuture<>(); + loaderQueue.add(new LoaderQueueEntry<>(key, future, loadContext)); + return future; + } else { + stats.incrementBatchLoadCountBy(1); + // immediate execution of batch function + return invokeLoaderImmediately(key, loadContext); + } + } CompletableFuture invokeLoaderImmediately(K key, Object keyContext) { List keys = singletonList(key); diff --git a/src/main/java/org/dataloader/DataLoaderOptions.java b/src/main/java/org/dataloader/DataLoaderOptions.java index ac54c3e..4a9d4d6 100644 --- a/src/main/java/org/dataloader/DataLoaderOptions.java +++ b/src/main/java/org/dataloader/DataLoaderOptions.java @@ -38,8 +38,9 @@ public class DataLoaderOptions { private boolean batchingEnabled; private boolean cachingEnabled; private boolean cachingExceptionsEnabled; - private CacheKey cacheKeyFunction; - private CacheMap cacheMap; + private CacheKey cacheKeyFunction; + private CacheMap cacheMap; + private CachedValueStore cachedValueStore; private int maxBatchSize; private Supplier statisticsCollector; private BatchLoaderContextProvider environmentProvider; @@ -166,7 +167,7 @@ public Optional cacheKeyFunction() { * * @return the data loader options for fluent coding */ - public DataLoaderOptions setCacheKeyFunction(CacheKey cacheKeyFunction) { + public DataLoaderOptions setCacheKeyFunction(CacheKey cacheKeyFunction) { this.cacheKeyFunction = cacheKeyFunction; return this; } @@ -178,7 +179,7 @@ public DataLoaderOptions setCacheKeyFunction(CacheKey cacheKeyFunction) { * * @return an optional with the cache map instance, or empty */ - public Optional cacheMap() { + public Optional> cacheMap() { return Optional.ofNullable(cacheMap); } @@ -189,7 +190,7 @@ public Optional cacheMap() { * * @return the data loader options for fluent coding */ - public DataLoaderOptions setCacheMap(CacheMap cacheMap) { + public DataLoaderOptions setCacheMap(CacheMap cacheMap) { this.cacheMap = cacheMap; return this; } @@ -256,4 +257,28 @@ public DataLoaderOptions setBatchLoaderContextProvider(BatchLoaderContextProvide this.environmentProvider = nonNull(contextProvider); return this; } + + /** + * Gets the (optional) cache store implementation that is used for value storage, if caching is enabled. + *

+ * If missing, a no-op implementation will be used. + * + * @return an optional with the cache store instance, or empty + */ + public Optional> cachedValueStore() { + return Optional.ofNullable(cachedValueStore); + } + + /** + * Sets the value store implementation to use for caching values, if caching is enabled. + * + * @param cachedValueStore the cache store instance + * + * @return the data loader options for fluent coding + */ + public DataLoaderOptions setCachedValueStore(CachedValueStore cachedValueStore) { + this.cachedValueStore = cachedValueStore; + return this; + } + } diff --git a/src/main/java/org/dataloader/impl/DefaultCacheMap.java b/src/main/java/org/dataloader/impl/DefaultCacheMap.java index 7245ce2..fe6baa0 100644 --- a/src/main/java/org/dataloader/impl/DefaultCacheMap.java +++ b/src/main/java/org/dataloader/impl/DefaultCacheMap.java @@ -21,19 +21,20 @@ import java.util.HashMap; import java.util.Map; +import java.util.concurrent.CompletableFuture; /** * Default implementation of {@link CacheMap} that is based on a regular {@link java.util.LinkedHashMap}. * - * @param type parameter indicating the type of the cache keys + * @param type parameter indicating the type of the cache keys * @param type parameter indicating the type of the data that is cached * * @author Arnold Schrijver */ @Internal -public class DefaultCacheMap implements CacheMap { +public class DefaultCacheMap implements CacheMap { - private final Map cache; + private final Map> cache; /** * Default constructor @@ -46,15 +47,16 @@ public DefaultCacheMap() { * {@inheritDoc} */ @Override - public boolean containsKey(U key) { + public boolean containsKey(K key) { return cache.containsKey(key); } + /** * {@inheritDoc} */ @Override - public V get(U key) { + public CompletableFuture get(K key) { return cache.get(key); } @@ -62,7 +64,7 @@ public V get(U key) { * {@inheritDoc} */ @Override - public CacheMap set(U key, V value) { + public CacheMap set(K key, CompletableFuture value) { cache.put(key, value); return this; } @@ -71,7 +73,7 @@ public CacheMap set(U key, V value) { * {@inheritDoc} */ @Override - public CacheMap delete(U key) { + public CacheMap delete(K key) { cache.remove(key); return this; } @@ -80,7 +82,7 @@ public CacheMap delete(U key) { * {@inheritDoc} */ @Override - public CacheMap clear() { + public CacheMap clear() { cache.clear(); return this; } diff --git a/src/main/java/org/dataloader/impl/DefaultCachedValueStore.java b/src/main/java/org/dataloader/impl/DefaultCachedValueStore.java new file mode 100644 index 0000000..f849525 --- /dev/null +++ b/src/main/java/org/dataloader/impl/DefaultCachedValueStore.java @@ -0,0 +1,62 @@ +package org.dataloader.impl; + + +import org.dataloader.CachedValueStore; +import org.dataloader.annotations.Internal; + +import java.util.concurrent.CompletableFuture; + +/** + * Default implementation of {@link CachedValueStore} that does nothing. + *

+ * We don't want to store values in memory twice, so when using the default store we just + * say we never have the key and complete the other methods by doing nothing. + * + * @param the type of cache keys + * @param the type of cache values + * + * @author Craig Day + */ +@Internal +public class DefaultCachedValueStore implements CachedValueStore { + + /** + * {@inheritDoc} + */ + @Override + public CompletableFuture containsKey(K key) { + return CompletableFuture.completedFuture(false); + } + + /** + * {@inheritDoc} + */ + @Override + public CompletableFuture get(K key) { + return CompletableFutureKit.failedFuture(new UnsupportedOperationException()); + } + + /** + * {@inheritDoc} + */ + @Override + public CompletableFuture set(K key, V value) { + return CompletableFuture.completedFuture(value); + } + + /** + * {@inheritDoc} + */ + @Override + public CompletableFuture delete(K key) { + return CompletableFuture.completedFuture(null); + } + + /** + * {@inheritDoc} + */ + @Override + public CompletableFuture clear() { + return CompletableFuture.completedFuture(null); + } +} \ No newline at end of file diff --git a/src/test/java/ReadmeExamples.java b/src/test/java/ReadmeExamples.java index ccdd555..6cef375 100644 --- a/src/test/java/ReadmeExamples.java +++ b/src/test/java/ReadmeExamples.java @@ -214,12 +214,12 @@ public boolean containsKey(Object key) { } @Override - public Object get(Object key) { + public CompletableFuture get(Object key) { return null; } @Override - public CacheMap set(Object key, Object value) { + public CacheMap set(Object key, CompletableFuture value) { return null; } diff --git a/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java b/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java new file mode 100644 index 0000000..463b72b --- /dev/null +++ b/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java @@ -0,0 +1,246 @@ +package org.dataloader; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import org.dataloader.fixtures.CaffeineCachedValueStore; +import org.dataloader.fixtures.CustomCachedValueStore; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; + +import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static java.util.concurrent.CompletableFuture.completedFuture; +import static org.awaitility.Awaitility.await; +import static org.dataloader.DataLoaderOptions.newOptions; +import static org.dataloader.fixtures.TestKit.idLoader; +import static org.dataloader.impl.CompletableFutureKit.failedFuture; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + +public class DataLoaderCachedValueStoreTest { + + @Test + public void test_by_default_we_have_no_value_caching() { + List> loadCalls = new ArrayList<>(); + DataLoaderOptions options = newOptions(); + DataLoader identityLoader = idLoader(options, loadCalls); + + CompletableFuture fA = identityLoader.load("a"); + CompletableFuture fB = identityLoader.load("b"); + + assertFalse(fA.isDone()); + assertFalse(fB.isDone()); + + await().until(identityLoader.dispatch()::isDone); + assertThat(fA.join(), equalTo("a")); + assertThat(fB.join(), equalTo("b")); + + assertThat(loadCalls, equalTo(singletonList(asList("a", "b")))); + + // futures are still cached but not values + + fA = identityLoader.load("a"); + fB = identityLoader.load("b"); + + assertTrue(fA.isDone()); + assertTrue(fB.isDone()); + + await().until(identityLoader.dispatch()::isDone); + assertThat(fA.join(), equalTo("a")); + assertThat(fB.join(), equalTo("b")); + + assertThat(loadCalls, equalTo(singletonList(asList("a", "b")))); + + } + + @Test + public void should_accept_a_remote_value_store_for_caching() { + CustomCachedValueStore customStore = new CustomCachedValueStore(); + List> loadCalls = new ArrayList<>(); + DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoader identityLoader = idLoader(options, loadCalls); + + // Fetches as expected + + CompletableFuture fA = identityLoader.load("a"); + CompletableFuture fB = identityLoader.load("b"); + + await().until(identityLoader.dispatch()::isDone); + assertThat(fA.join(), equalTo("a")); + assertThat(fB.join(), equalTo("b")); + + assertThat(loadCalls, equalTo(singletonList(asList("a", "b")))); + assertArrayEquals(customStore.store.keySet().toArray(), asList("a", "b").toArray()); + + CompletableFuture future3 = identityLoader.load("c"); + CompletableFuture future2a = identityLoader.load("b"); + + await().until(identityLoader.dispatch()::isDone); + assertThat(future3.join(), equalTo("c")); + assertThat(future2a.join(), equalTo("b")); + + assertThat(loadCalls, equalTo(asList(asList("a", "b"), singletonList("c")))); + assertArrayEquals(customStore.store.keySet().toArray(), asList("a", "b", "c").toArray()); + + // Supports clear + + CompletableFuture fC = new CompletableFuture<>(); + identityLoader.clear("b", (v, e) -> fC.complete(v)); + await().until(fC::isDone); + assertArrayEquals(customStore.store.keySet().toArray(), asList("a", "c").toArray()); + + // Supports clear all + + CompletableFuture fCa = new CompletableFuture<>(); + identityLoader.clearAll((v, e) -> fCa.complete(v)); + await().until(fCa::isDone); + assertArrayEquals(customStore.store.keySet().toArray(), emptyList().toArray()); + } + + @Test + public void can_use_caffeine_for_caching() { + // + // Mostly to prove that some other CACHE library could be used + // as the backing value cache. Not really Caffeine specific. + // + Cache caffeineCache = Caffeine.newBuilder() + .expireAfterWrite(10, TimeUnit.MINUTES) + .maximumSize(100) + .build(); + + CachedValueStore customStore = new CaffeineCachedValueStore(caffeineCache); + + List> loadCalls = new ArrayList<>(); + DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoader identityLoader = idLoader(options, loadCalls); + + // Fetches as expected + + CompletableFuture fA = identityLoader.load("a"); + CompletableFuture fB = identityLoader.load("b"); + + await().until(identityLoader.dispatch()::isDone); + assertThat(fA.join(), equalTo("a")); + assertThat(fB.join(), equalTo("b")); + + assertThat(loadCalls, equalTo(singletonList(asList("a", "b")))); + assertArrayEquals(caffeineCache.asMap().keySet().toArray(), asList("a", "b").toArray()); + + CompletableFuture fC = identityLoader.load("c"); + CompletableFuture fBa = identityLoader.load("b"); + + await().until(identityLoader.dispatch()::isDone); + assertThat(fC.join(), equalTo("c")); + assertThat(fBa.join(), equalTo("b")); + + assertThat(loadCalls, equalTo(asList(asList("a", "b"), singletonList("c")))); + assertArrayEquals(caffeineCache.asMap().keySet().toArray(), asList("a", "b", "c").toArray()); + } + + @Test + public void will_invoke_loader_if_CACHE_CONTAINS_call_throws_exception() { + CustomCachedValueStore customStore = new CustomCachedValueStore() { + @Override + public CompletableFuture containsKey(String key) { + if (key.equals("a")) { + return failedFuture(new IllegalStateException("no A")); + } + if (key.equals("c")) { + return completedFuture(false); + } + return super.containsKey(key); + } + }; + customStore.set("a", "Not From Cache A"); + customStore.set("b", "From Cache"); + customStore.set("c", "Not From Cache C"); + + List> loadCalls = new ArrayList<>(); + DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoader identityLoader = idLoader(options, loadCalls); + + // Fetches as expected + + CompletableFuture fA = identityLoader.load("a"); + CompletableFuture fB = identityLoader.load("b"); + CompletableFuture fC = identityLoader.load("c"); + + await().until(identityLoader.dispatch()::isDone); + assertThat(fA.join(), equalTo("a")); + assertThat(fB.join(), equalTo("From Cache")); + assertThat(fC.join(), equalTo("c")); + + // a was not in cache (according to containsKey) and hence needed to be loaded + assertThat(loadCalls, equalTo(singletonList(asList("a", "c")))); + + // the failed containsKey calls will be SET back into the value cache after batch loading + assertArrayEquals(customStore.store.keySet().toArray(), asList("a", "b", "c").toArray()); + assertArrayEquals(customStore.store.values().toArray(), asList("a", "From Cache", "c").toArray()); + } + + @Test + public void will_invoke_loader_if_CACHE_GET_call_throws_exception() { + CustomCachedValueStore customStore = new CustomCachedValueStore() { + + @Override + public CompletableFuture get(String key) { + if (key.equals("a")) { + return failedFuture(new IllegalStateException("no A")); + } + return super.get(key); + } + }; + customStore.set("a", "Not From Cache"); + customStore.set("b", "From Cache"); + + List> loadCalls = new ArrayList<>(); + DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoader identityLoader = idLoader(options, loadCalls); + + CompletableFuture fA = identityLoader.load("a"); + CompletableFuture fB = identityLoader.load("b"); + + await().until(identityLoader.dispatch()::isDone); + assertThat(fA.join(), equalTo("a")); + assertThat(fB.join(), equalTo("From Cache")); + + // a was not in cache (according to get) and hence needed to be loaded + assertThat(loadCalls, equalTo(singletonList(singletonList("a")))); + } + + @Test + public void will_still_work_if_CACHE_SET_call_throws_exception() { + CustomCachedValueStore customStore = new CustomCachedValueStore() { + @Override + public CompletableFuture set(String key, Object value) { + if (key.equals("a")) { + return failedFuture(new IllegalStateException("no A")); + } + return super.set(key, value); + } + }; + + List> loadCalls = new ArrayList<>(); + DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoader identityLoader = idLoader(options, loadCalls); + + CompletableFuture fA = identityLoader.load("a"); + CompletableFuture fB = identityLoader.load("b"); + + await().until(identityLoader.dispatch()::isDone); + assertThat(fA.join(), equalTo("a")); + assertThat(fB.join(), equalTo("b")); + + // a was not in cache (according to get) and hence needed to be loaded + assertThat(loadCalls, equalTo(singletonList(asList("a", "b")))); + assertArrayEquals(customStore.store.keySet().toArray(), singletonList("b").toArray()); + } +} diff --git a/src/test/java/org/dataloader/DataLoaderTest.java b/src/test/java/org/dataloader/DataLoaderTest.java index 18aa1ba..08d5b44 100644 --- a/src/test/java/org/dataloader/DataLoaderTest.java +++ b/src/test/java/org/dataloader/DataLoaderTest.java @@ -16,6 +16,7 @@ package org.dataloader; +import org.dataloader.fixtures.CustomCacheMap; import org.dataloader.fixtures.JsonObject; import org.dataloader.fixtures.TestKit; import org.dataloader.fixtures.User; diff --git a/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java b/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java new file mode 100644 index 0000000..6960859 --- /dev/null +++ b/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java @@ -0,0 +1,51 @@ +package org.dataloader.fixtures; + + +import com.github.benmanes.caffeine.cache.Cache; +import org.dataloader.CachedValueStore; +import org.dataloader.impl.CompletableFutureKit; + +import java.util.concurrent.CompletableFuture; + +public class CaffeineCachedValueStore implements CachedValueStore { + + public final Cache cache; + + public CaffeineCachedValueStore(Cache cache) { + this.cache = cache; + } + + @Override + public CompletableFuture containsKey(String key) { + // caffeine cant answer this question efficiently so we rely on + return CompletableFuture.completedFuture(true); + } + + @Override + public CompletableFuture get(String key) { + Object value = cache.getIfPresent(key); + if (value == null) { + // we use get exceptions here to indicate not in cache + return CompletableFutureKit.failedFuture(new RuntimeException(key + " not present")); + } + return CompletableFuture.completedFuture(value); + } + + @Override + public CompletableFuture set(String key, Object value) { + cache.put(key, value); + return CompletableFuture.completedFuture(value); + } + + @Override + public CompletableFuture delete(String key) { + cache.invalidate(key); + return CompletableFuture.completedFuture(null); + } + + @Override + public CompletableFuture clear() { + cache.invalidateAll(); + return CompletableFuture.completedFuture(null); + } +} \ No newline at end of file diff --git a/src/test/java/org/dataloader/CustomCacheMap.java b/src/test/java/org/dataloader/fixtures/CustomCacheMap.java similarity index 68% rename from src/test/java/org/dataloader/CustomCacheMap.java rename to src/test/java/org/dataloader/fixtures/CustomCacheMap.java index 505148d..51e6687 100644 --- a/src/test/java/org/dataloader/CustomCacheMap.java +++ b/src/test/java/org/dataloader/fixtures/CustomCacheMap.java @@ -1,11 +1,14 @@ -package org.dataloader; +package org.dataloader.fixtures; + +import org.dataloader.CacheMap; import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.CompletableFuture; public class CustomCacheMap implements CacheMap { - public Map stash; + public Map> stash; public CustomCacheMap() { stash = new LinkedHashMap<>(); @@ -17,12 +20,12 @@ public boolean containsKey(String key) { } @Override - public Object get(String key) { + public CompletableFuture get(String key) { return stash.get(key); } @Override - public CacheMap set(String key, Object value) { + public CacheMap set(String key, CompletableFuture value) { stash.put(key, value); return this; } diff --git a/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java b/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java new file mode 100644 index 0000000..56419e5 --- /dev/null +++ b/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java @@ -0,0 +1,41 @@ +package org.dataloader.fixtures; + + +import org.dataloader.CachedValueStore; + +import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; + +public class CustomCachedValueStore implements CachedValueStore { + + public final Map store = new ConcurrentHashMap<>(); + + @Override + public CompletableFuture containsKey(String key) { + return CompletableFuture.completedFuture(store.containsKey(key)); + } + + @Override + public CompletableFuture get(String key) { + return CompletableFuture.completedFuture(store.get(key)); + } + + @Override + public CompletableFuture set(String key, Object value) { + store.put(key, value); + return CompletableFuture.completedFuture(value); + } + + @Override + public CompletableFuture delete(String key) { + store.remove(key); + return CompletableFuture.completedFuture(null); + } + + @Override + public CompletableFuture clear() { + store.clear(); + return CompletableFuture.completedFuture(null); + } +} \ No newline at end of file From d651b94dbd02a72c726531ee82767f41a5220310 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 28 Jun 2021 23:12:24 +1000 Subject: [PATCH 2/5] Readme updates as well as a rename of the default value store to NoOp --- README.md | 68 +++++++++++++------ .../java/org/dataloader/CachedValueStore.java | 6 +- .../org/dataloader/impl/DefaultCacheMap.java | 2 +- ...ueStore.java => NoOpCachedValueStore.java} | 6 +- 4 files changed, 56 insertions(+), 26 deletions(-) rename src/main/java/org/dataloader/impl/{DefaultCachedValueStore.java => NoOpCachedValueStore.java} (85%) diff --git a/README.md b/README.md index 73351ad..fcedd6b 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,7 @@ a list of user ids in one call. This is important consideration. By using `dataloader` you have batched up the requests for N keys in a list of keys that can be retrieved at one time. - If you don't have batched backing services, then you cant be as efficient as possible as you will have to make N calls for each key. + If you don't have batched backing services, then you can't be as efficient as possible as you will have to make N calls for each key. ```java BatchLoader lessEfficientUserBatchLoader = new BatchLoader() { @@ -313,6 +313,47 @@ and some of which may have failed. From that data loader can infer the right be On the above example if one of the `Try` objects represents a failure, then its `load()` promise will complete exceptionally and you can react to that, in a type safe manner. +## Caching + +`DataLoader` has a two tiered caching system in place. + +The first cache is represented by the interface `org.dataloader.CacheMap`. It will cache `CompletableFuture`s by key and hence future `load(key)` calls +will be given the same future and hence the same value. + +This cache can only work local to the JVM, since its caches `CompletableFuture`s which cannot be serialised across a network say. + +The second level cache is a value cache represented by the interface `org.dataloader.CachedValueStore`. By default, this is not enabled and is a no-op. + +The value cache uses an async API pattern to encapsulate the idea that the value cache could be in a remote place such as REDIS or Memcached. + +## Custom future caches + +The default cache behind `DataLoader` is an in memory `HashMap`. There is no expiry on this, and it lives for as long as the data loader +lives. + +However, you can create your own custom cache and supply it to the data loader on construction via the `org.dataloader.CacheMap` interface. + +```java + MyCustomCache customCache = new MyCustomCache(); + DataLoaderOptions options = DataLoaderOptions.newOptions().setCacheMap(customCache); + DataLoaderFactory.newDataLoader(userBatchLoader, options); +``` + +You could choose to use one of the fancy cache implementations from Guava or Caffeine and wrap it in a `CacheMap` wrapper ready +for data loader. They can do fancy things like time eviction and efficient LRU caching. + +As stated above, a custom `org.dataloader.CacheMap` is a local cache of futures with values, not values per se. + +## Custom value caches + +You will need to create your own implementations of the `org.dataloader.CachedValueStore` if your want to use an external cache. + +This library does not ship with any implementations of `CachedValueStore` because it does not want to have +production dependencies on external cache libraries. + +The API of `CachedValueStore` has been designed to be asynchronous because it is expected that the value cache could be outside +your JVM. It uses `Future`s to get and set values into cache, which may involve a network call and hence exceptional failures to get +or set values. ## Disabling caching @@ -346,7 +387,7 @@ More complex cache behavior can be achieved by calling `.clear()` or `.clearAll( ## Caching errors If a batch load fails (that is, a batch function returns a rejected CompletionStage), then the requested values will not be cached. -However if a batch function returns a `Try` or `Throwable` instance for an individual value, then that will be cached to avoid frequently loading +However, if a batch function returns a `Try` or `Throwable` instance for an individual value, then that will be cached to avoid frequently loading the same problem object. In some circumstances you may wish to clear the cache for these individual problems: @@ -406,33 +447,18 @@ If your data can be shared across web requests then use a custom cache to keep v Data loaders are stateful components that contain promises (with context) that are likely share the same affinity as the request. -## Custom caches - -The default cache behind `DataLoader` is an in memory `HashMap`. There is no expiry on this, and it lives for as long as the data loader -lives. - -However, you can create your own custom cache and supply it to the data loader on construction via the `org.dataloader.CacheMap` interface. - -```java - MyCustomCache customCache = new MyCustomCache(); - DataLoaderOptions options = DataLoaderOptions.newOptions().setCacheMap(customCache); - DataLoaderFactory.newDataLoader(userBatchLoader, options); -``` - -You could choose to use one of the fancy cache implementations from Guava or Kaffeine and wrap it in a `CacheMap` wrapper ready -for data loader. They can do fancy things like time eviction and efficient LRU caching. - ## Manual dispatching -The original [Facebook DataLoader](https://github.com/facebook/dataloader) was written in Javascript for NodeJS. NodeJS is single-threaded in nature, but simulates -asynchronous logic by invoking functions on separate threads in an event loop, as explained +The original [Facebook DataLoader](https://github.com/facebook/dataloader) was written in Javascript for NodeJS. + +NodeJS is single-threaded in nature, but simulates asynchronous logic by invoking functions on separate threads in an event loop, as explained [in this post](http://stackoverflow.com/a/19823583/3455094) on StackOverflow. NodeJS generates so-call 'ticks' in which queued functions are dispatched for execution, and Facebook `DataLoader` uses the `nextTick()` function in NodeJS to _automatically_ dequeue load requests and send them to the batch execution function for processing. -And here there is an **IMPORTANT DIFFERENCE** compared to how `java-dataloader` operates!! +Here there is an **IMPORTANT DIFFERENCE** compared to how `java-dataloader` operates!! In NodeJS the batch preparation will not affect the asynchronous processing behaviour in any way. It will just prepare batches in 'spare time' as it were. diff --git a/src/main/java/org/dataloader/CachedValueStore.java b/src/main/java/org/dataloader/CachedValueStore.java index 2135330..62584b8 100644 --- a/src/main/java/org/dataloader/CachedValueStore.java +++ b/src/main/java/org/dataloader/CachedValueStore.java @@ -1,7 +1,7 @@ package org.dataloader; import org.dataloader.annotations.PublicSpi; -import org.dataloader.impl.DefaultCachedValueStore; +import org.dataloader.impl.NoOpCachedValueStore; import java.util.List; import java.util.concurrent.CompletableFuture; @@ -24,6 +24,7 @@ @PublicSpi public interface CachedValueStore { + /** * Creates a new store, using the default no-op implementation. * @@ -33,7 +34,8 @@ public interface CachedValueStore { * @return the cache store */ static CachedValueStore defaultStore() { - return new DefaultCachedValueStore<>(); + //noinspection unchecked + return (CachedValueStore) NoOpCachedValueStore.NOOP; } /** diff --git a/src/main/java/org/dataloader/impl/DefaultCacheMap.java b/src/main/java/org/dataloader/impl/DefaultCacheMap.java index fe6baa0..9346ad8 100644 --- a/src/main/java/org/dataloader/impl/DefaultCacheMap.java +++ b/src/main/java/org/dataloader/impl/DefaultCacheMap.java @@ -24,7 +24,7 @@ import java.util.concurrent.CompletableFuture; /** - * Default implementation of {@link CacheMap} that is based on a regular {@link java.util.LinkedHashMap}. + * Default implementation of {@link CacheMap} that is based on a regular {@link java.util.HashMap}. * * @param type parameter indicating the type of the cache keys * @param type parameter indicating the type of the data that is cached diff --git a/src/main/java/org/dataloader/impl/DefaultCachedValueStore.java b/src/main/java/org/dataloader/impl/NoOpCachedValueStore.java similarity index 85% rename from src/main/java/org/dataloader/impl/DefaultCachedValueStore.java rename to src/main/java/org/dataloader/impl/NoOpCachedValueStore.java index f849525..ce07b9d 100644 --- a/src/main/java/org/dataloader/impl/DefaultCachedValueStore.java +++ b/src/main/java/org/dataloader/impl/NoOpCachedValueStore.java @@ -7,7 +7,7 @@ import java.util.concurrent.CompletableFuture; /** - * Default implementation of {@link CachedValueStore} that does nothing. + * Implementation of {@link CachedValueStore} that does nothing. *

* We don't want to store values in memory twice, so when using the default store we just * say we never have the key and complete the other methods by doing nothing. @@ -18,7 +18,9 @@ * @author Craig Day */ @Internal -public class DefaultCachedValueStore implements CachedValueStore { +public class NoOpCachedValueStore implements CachedValueStore { + + public static NoOpCachedValueStore NOOP = new NoOpCachedValueStore<>(); /** * {@inheritDoc} From ccbf15a97643dac0fddfccf25a2e7a5e02f6ab8e Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Wed, 30 Jun 2021 14:47:15 +1000 Subject: [PATCH 3/5] PR feedback - removed containsKey, updated a few method names and updated doc --- .../java/org/dataloader/CachedValueStore.java | 26 +++--------- src/main/java/org/dataloader/DataLoader.java | 10 ++--- .../java/org/dataloader/DataLoaderHelper.java | 14 ++----- .../dataloader/impl/NoOpCachedValueStore.java | 8 ---- .../DataLoaderCachedValueStoreTest.java | 41 ------------------- .../fixtures/CaffeineCachedValueStore.java | 6 --- .../fixtures/CustomCachedValueStore.java | 9 ++-- 7 files changed, 17 insertions(+), 97 deletions(-) diff --git a/src/main/java/org/dataloader/CachedValueStore.java b/src/main/java/org/dataloader/CachedValueStore.java index 62584b8..c8ed7f8 100644 --- a/src/main/java/org/dataloader/CachedValueStore.java +++ b/src/main/java/org/dataloader/CachedValueStore.java @@ -3,7 +3,6 @@ import org.dataloader.annotations.PublicSpi; import org.dataloader.impl.NoOpCachedValueStore; -import java.util.List; import java.util.concurrent.CompletableFuture; /** @@ -33,36 +32,21 @@ public interface CachedValueStore { * * @return the cache store */ - static CachedValueStore defaultStore() { + static CachedValueStore defaultCachedValueStore() { //noinspection unchecked return (CachedValueStore) NoOpCachedValueStore.NOOP; } /** - * Checks whether the specified key is contained in the store. - *

- * {@link DataLoader} first calls {@link #containsKey(Object)} and then calls {@link #get(Object)}. If the - * backing cache implementation cannot answer the `containsKey` call then simply return true and the - * following `get` call can complete exceptionally to cause the {@link DataLoader} - * to enqueue the key to the {@link BatchLoader#load(List)} call since it is not present in cache. - * - * @param key the key to check if its present in the cache - * - * @return {@code true} if the cache contains the key, {@code false} otherwise - */ - CompletableFuture containsKey(K key); - - /** - * Gets the specified key from the store. + * Gets the specified key from the store. if the key si not present, then the implementation MUST return an exceptionally completed future + * and not null because null is a valid cacheable value. Any exception is will cause {@link DataLoader} to load the key via batch loading + * instead. * * @param key the key to retrieve * - * @return a future containing the cached value (which maybe null) or an exception if the key does + * @return a future containing the cached value (which maybe null) or exceptionally completed future if the key does * not exist in the cache. * - * IMPORTANT: The future may fail if the key does not exist depending on implementation. Implementations should - * return an exceptional future if the key is not present in the cache, not null which is a valid value - * for a key. */ CompletableFuture get(K key); diff --git a/src/main/java/org/dataloader/DataLoader.java b/src/main/java/org/dataloader/DataLoader.java index 8ed5b13..6beae55 100644 --- a/src/main/java/org/dataloader/DataLoader.java +++ b/src/main/java/org/dataloader/DataLoader.java @@ -415,8 +415,8 @@ public DataLoader(BatchLoader batchLoadFunction, DataLoaderOptions options @VisibleForTesting DataLoader(Object batchLoadFunction, DataLoaderOptions options, Clock clock) { DataLoaderOptions loaderOptions = options == null ? new DataLoaderOptions() : options; - this.futureCache = determineCacheMap(loaderOptions); - this.cachedValueStore = determineCacheStore(loaderOptions); + this.futureCache = determineFutureCache(loaderOptions); + this.cachedValueStore = determineCachedValueStore(loaderOptions); // order of keys matter in data loader this.stats = nonNull(loaderOptions.getStatisticsCollector()); @@ -425,13 +425,13 @@ public DataLoader(BatchLoader batchLoadFunction, DataLoaderOptions options @SuppressWarnings("unchecked") - private CacheMap determineCacheMap(DataLoaderOptions loaderOptions) { + private CacheMap determineFutureCache(DataLoaderOptions loaderOptions) { return (CacheMap) loaderOptions.cacheMap().orElseGet(CacheMap::simpleMap); } @SuppressWarnings("unchecked") - private CachedValueStore determineCacheStore(DataLoaderOptions loaderOptions) { - return (CachedValueStore) loaderOptions.cachedValueStore().orElseGet(CachedValueStore::defaultStore); + private CachedValueStore determineCachedValueStore(DataLoaderOptions loaderOptions) { + return (CachedValueStore) loaderOptions.cachedValueStore().orElseGet(CachedValueStore::defaultCachedValueStore); } /** diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java index af3417e..c97a710 100644 --- a/src/main/java/org/dataloader/DataLoaderHelper.java +++ b/src/main/java/org/dataloader/DataLoaderHelper.java @@ -307,17 +307,9 @@ private CompletableFuture loadFromCache(K key, Object loadContext, boolean ba */ final CompletableFuture future = new CompletableFuture<>(); - cachedValueStore.containsKey(cacheKey).whenComplete((hasKey, containsCallEx) -> { - boolean containsKey = containsCallEx == null && Boolean.TRUE.equals(hasKey); - if (containsKey) { - cachedValueStore.get(cacheKey).whenComplete((cachedValue, getCallEx) -> { - if (getCallEx == null) { - future.complete(cachedValue); - } else { - queueOrInvokeLoader(key, loadContext, batchingEnabled) - .whenComplete(setValueIntoCacheAndCompleteFuture(cacheKey, future)); - } - }); + cachedValueStore.get(cacheKey).whenComplete((cachedValue, getCallEx) -> { + if (getCallEx == null) { + future.complete(cachedValue); } else { queueOrInvokeLoader(key, loadContext, batchingEnabled) .whenComplete(setValueIntoCacheAndCompleteFuture(cacheKey, future)); diff --git a/src/main/java/org/dataloader/impl/NoOpCachedValueStore.java b/src/main/java/org/dataloader/impl/NoOpCachedValueStore.java index ce07b9d..0d04616 100644 --- a/src/main/java/org/dataloader/impl/NoOpCachedValueStore.java +++ b/src/main/java/org/dataloader/impl/NoOpCachedValueStore.java @@ -22,14 +22,6 @@ public class NoOpCachedValueStore implements CachedValueStore { public static NoOpCachedValueStore NOOP = new NoOpCachedValueStore<>(); - /** - * {@inheritDoc} - */ - @Override - public CompletableFuture containsKey(K key) { - return CompletableFuture.completedFuture(false); - } - /** * {@inheritDoc} */ diff --git a/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java b/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java index 463b72b..8f135ca 100644 --- a/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java +++ b/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java @@ -145,47 +145,6 @@ public void can_use_caffeine_for_caching() { assertArrayEquals(caffeineCache.asMap().keySet().toArray(), asList("a", "b", "c").toArray()); } - @Test - public void will_invoke_loader_if_CACHE_CONTAINS_call_throws_exception() { - CustomCachedValueStore customStore = new CustomCachedValueStore() { - @Override - public CompletableFuture containsKey(String key) { - if (key.equals("a")) { - return failedFuture(new IllegalStateException("no A")); - } - if (key.equals("c")) { - return completedFuture(false); - } - return super.containsKey(key); - } - }; - customStore.set("a", "Not From Cache A"); - customStore.set("b", "From Cache"); - customStore.set("c", "Not From Cache C"); - - List> loadCalls = new ArrayList<>(); - DataLoaderOptions options = newOptions().setCachedValueStore(customStore); - DataLoader identityLoader = idLoader(options, loadCalls); - - // Fetches as expected - - CompletableFuture fA = identityLoader.load("a"); - CompletableFuture fB = identityLoader.load("b"); - CompletableFuture fC = identityLoader.load("c"); - - await().until(identityLoader.dispatch()::isDone); - assertThat(fA.join(), equalTo("a")); - assertThat(fB.join(), equalTo("From Cache")); - assertThat(fC.join(), equalTo("c")); - - // a was not in cache (according to containsKey) and hence needed to be loaded - assertThat(loadCalls, equalTo(singletonList(asList("a", "c")))); - - // the failed containsKey calls will be SET back into the value cache after batch loading - assertArrayEquals(customStore.store.keySet().toArray(), asList("a", "b", "c").toArray()); - assertArrayEquals(customStore.store.values().toArray(), asList("a", "From Cache", "c").toArray()); - } - @Test public void will_invoke_loader_if_CACHE_GET_call_throws_exception() { CustomCachedValueStore customStore = new CustomCachedValueStore() { diff --git a/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java b/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java index 6960859..14520a3 100644 --- a/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java +++ b/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java @@ -15,12 +15,6 @@ public CaffeineCachedValueStore(Cache cache) { this.cache = cache; } - @Override - public CompletableFuture containsKey(String key) { - // caffeine cant answer this question efficiently so we rely on - return CompletableFuture.completedFuture(true); - } - @Override public CompletableFuture get(String key) { Object value = cache.getIfPresent(key); diff --git a/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java b/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java index 56419e5..e184b0c 100644 --- a/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java +++ b/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java @@ -2,6 +2,7 @@ import org.dataloader.CachedValueStore; +import org.dataloader.impl.CompletableFutureKit; import java.util.Map; import java.util.concurrent.CompletableFuture; @@ -11,13 +12,11 @@ public class CustomCachedValueStore implements CachedValueStore public final Map store = new ConcurrentHashMap<>(); - @Override - public CompletableFuture containsKey(String key) { - return CompletableFuture.completedFuture(store.containsKey(key)); - } - @Override public CompletableFuture get(String key) { + if (!store.containsKey(key)) { + return CompletableFutureKit.failedFuture(new RuntimeException("The key is missing")); + } return CompletableFuture.completedFuture(store.get(key)); } From 161adecb2285b0a9aa91f60035899a441b314261 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 5 Jul 2021 20:39:22 +1000 Subject: [PATCH 4/5] PR feedback - name of ValueCache --- README.md | 16 ++++++------ src/main/java/org/dataloader/CacheMap.java | 13 +++++----- src/main/java/org/dataloader/DataLoader.java | 16 ++++++------ .../java/org/dataloader/DataLoaderHelper.java | 10 ++++---- .../org/dataloader/DataLoaderOptions.java | 17 ++++++------- ...{CachedValueStore.java => ValueCache.java} | 25 +++++++++++++------ ...hedValueStore.java => NoOpValueCache.java} | 8 +++--- ...est.java => DataLoaderValueCacheTest.java} | 23 ++++++++--------- ...alueStore.java => CaffeineValueCache.java} | 6 ++--- ...dValueStore.java => CustomValueCache.java} | 4 +-- 10 files changed, 73 insertions(+), 65 deletions(-) rename src/main/java/org/dataloader/{CachedValueStore.java => ValueCache.java} (61%) rename src/main/java/org/dataloader/impl/{NoOpCachedValueStore.java => NoOpValueCache.java} (81%) rename src/test/java/org/dataloader/{DataLoaderCachedValueStoreTest.java => DataLoaderValueCacheTest.java} (89%) rename src/test/java/org/dataloader/fixtures/{CaffeineCachedValueStore.java => CaffeineValueCache.java} (85%) rename src/test/java/org/dataloader/fixtures/{CustomCachedValueStore.java => CustomValueCache.java} (89%) diff --git a/README.md b/README.md index fcedd6b..4263dff 100644 --- a/README.md +++ b/README.md @@ -322,13 +322,13 @@ will be given the same future and hence the same value. This cache can only work local to the JVM, since its caches `CompletableFuture`s which cannot be serialised across a network say. -The second level cache is a value cache represented by the interface `org.dataloader.CachedValueStore`. By default, this is not enabled and is a no-op. +The second level cache is a value cache represented by the interface `org.dataloader.ValueCache`. By default, this is not enabled and is a no-op. The value cache uses an async API pattern to encapsulate the idea that the value cache could be in a remote place such as REDIS or Memcached. ## Custom future caches -The default cache behind `DataLoader` is an in memory `HashMap`. There is no expiry on this, and it lives for as long as the data loader +The default future cache behind `DataLoader` is an in memory `HashMap`. There is no expiry on this, and it lives for as long as the data loader lives. However, you can create your own custom cache and supply it to the data loader on construction via the `org.dataloader.CacheMap` interface. @@ -346,13 +346,15 @@ As stated above, a custom `org.dataloader.CacheMap` is a local cache of futures ## Custom value caches -You will need to create your own implementations of the `org.dataloader.CachedValueStore` if your want to use an external cache. +You will need to create your own implementations of the `org.dataloader.ValueCache` if your want to use an external cache. -This library does not ship with any implementations of `CachedValueStore` because it does not want to have -production dependencies on external cache libraries. +This library does not ship with any implementations of `ValueCache` because it does not want to have +production dependencies on external cache libraries, but you can easily write your own. -The API of `CachedValueStore` has been designed to be asynchronous because it is expected that the value cache could be outside -your JVM. It uses `Future`s to get and set values into cache, which may involve a network call and hence exceptional failures to get +The tests have an example based on [Caffeine](https://github.com/ben-manes/caffeine). + +The API of `ValueCache` has been designed to be asynchronous because it is expected that the value cache could be outside +your JVM. It uses `CompleteableFuture`s to get and set values into cache, which may involve a network call and hence exceptional failures to get or set values. diff --git a/src/main/java/org/dataloader/CacheMap.java b/src/main/java/org/dataloader/CacheMap.java index 9ae98f5..d2d0408 100644 --- a/src/main/java/org/dataloader/CacheMap.java +++ b/src/main/java/org/dataloader/CacheMap.java @@ -22,15 +22,14 @@ import java.util.concurrent.CompletableFuture; /** - * Cache map interface for data loaders that use caching. + * CacheMap is used by data loaders that use caching promises to values aka {@link CompletableFuture}<V>. A better name for this + * class might have been FutureCache but that is history now. *

- * The default implementation used by the data loader is based on a {@link java.util.LinkedHashMap}. Note that the - * implementation could also have used a regular {@link java.util.Map} instead of this {@link CacheMap}, but - * this aligns better to the reference data loader implementation provided by Facebook + * The default implementation used by the data loader is based on a {@link java.util.LinkedHashMap}. *

- * This is really a cache of completed {@link CompletableFuture} values in memory. It is used, when caching is enabled, to - * give back the same future to any code that may call it. if you need a cache of the underlying values that is possible external to the JVM - * then you will want to use {{@link CachedValueStore}} which is designed for external cache access. + * This is really a cache of completed {@link CompletableFuture}<V> values in memory. It is used, when caching is enabled, to + * give back the same future to any code that may call it. If you need a cache of the underlying values that is possible external to the JVM + * then you will want to use {{@link ValueCache}} which is designed for external cache access. * * @param type parameter indicating the type of the cache keys * @param type parameter indicating the type of the data that is cached diff --git a/src/main/java/org/dataloader/DataLoader.java b/src/main/java/org/dataloader/DataLoader.java index 6beae55..0aea1c7 100644 --- a/src/main/java/org/dataloader/DataLoader.java +++ b/src/main/java/org/dataloader/DataLoader.java @@ -67,7 +67,7 @@ public class DataLoader { private final DataLoaderHelper helper; private final StatisticsCollector stats; private final CacheMap futureCache; - private final CachedValueStore cachedValueStore; + private final ValueCache valueCache; /** * Creates new DataLoader with the specified batch loader function and default options @@ -416,11 +416,11 @@ public DataLoader(BatchLoader batchLoadFunction, DataLoaderOptions options DataLoader(Object batchLoadFunction, DataLoaderOptions options, Clock clock) { DataLoaderOptions loaderOptions = options == null ? new DataLoaderOptions() : options; this.futureCache = determineFutureCache(loaderOptions); - this.cachedValueStore = determineCachedValueStore(loaderOptions); + this.valueCache = determineValueCache(loaderOptions); // order of keys matter in data loader this.stats = nonNull(loaderOptions.getStatisticsCollector()); - this.helper = new DataLoaderHelper<>(this, batchLoadFunction, loaderOptions, this.futureCache, this.cachedValueStore, this.stats, clock); + this.helper = new DataLoaderHelper<>(this, batchLoadFunction, loaderOptions, this.futureCache, this.valueCache, this.stats, clock); } @@ -430,8 +430,8 @@ private CacheMap determineFutureCache(DataLoaderOptions loaderOptions } @SuppressWarnings("unchecked") - private CachedValueStore determineCachedValueStore(DataLoaderOptions loaderOptions) { - return (CachedValueStore) loaderOptions.cachedValueStore().orElseGet(CachedValueStore::defaultCachedValueStore); + private ValueCache determineValueCache(DataLoaderOptions loaderOptions) { + return (ValueCache) loaderOptions.valueCache().orElseGet(ValueCache::defaultValueCache); } /** @@ -652,7 +652,7 @@ public DataLoader clear(K key, BiConsumer handler) { Object cacheKey = getCacheKey(key); synchronized (this) { futureCache.delete(cacheKey); - cachedValueStore.delete(key).whenComplete(handler); + valueCache.delete(key).whenComplete(handler); } return this; } @@ -677,14 +677,14 @@ public DataLoader clearAll() { public DataLoader clearAll(BiConsumer handler) { synchronized (this) { futureCache.clear(); - cachedValueStore.clear().whenComplete(handler); + valueCache.clear().whenComplete(handler); } return this; } /** * Primes the cache with the given key and value. Note this will only prime the future cache - * and not the value store. Use {@link CachedValueStore#set(Object, Object)} if you want + * and not the value store. Use {@link ValueCache#set(Object, Object)} if you want * o prime it with values before use * * @param key the key diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java index c97a710..fab4f9b 100644 --- a/src/main/java/org/dataloader/DataLoaderHelper.java +++ b/src/main/java/org/dataloader/DataLoaderHelper.java @@ -63,7 +63,7 @@ Object getCallContext() { private final Object batchLoadFunction; private final DataLoaderOptions loaderOptions; private final CacheMap futureCache; - private final CachedValueStore cachedValueStore; + private final ValueCache valueCache; private final List>> loaderQueue; private final StatisticsCollector stats; private final Clock clock; @@ -73,14 +73,14 @@ Object getCallContext() { Object batchLoadFunction, DataLoaderOptions loaderOptions, CacheMap futureCache, - CachedValueStore cachedValueStore, + ValueCache valueCache, StatisticsCollector stats, Clock clock) { this.dataLoader = dataLoader; this.batchLoadFunction = batchLoadFunction; this.loaderOptions = loaderOptions; this.futureCache = futureCache; - this.cachedValueStore = cachedValueStore; + this.valueCache = valueCache; this.loaderQueue = new ArrayList<>(); this.stats = stats; this.clock = clock; @@ -307,7 +307,7 @@ private CompletableFuture loadFromCache(K key, Object loadContext, boolean ba */ final CompletableFuture future = new CompletableFuture<>(); - cachedValueStore.get(cacheKey).whenComplete((cachedValue, getCallEx) -> { + valueCache.get(cacheKey).whenComplete((cachedValue, getCallEx) -> { if (getCallEx == null) { future.complete(cachedValue); } else { @@ -324,7 +324,7 @@ private CompletableFuture loadFromCache(K key, Object loadContext, boolean ba private BiConsumer setValueIntoCacheAndCompleteFuture(Object cacheKey, CompletableFuture future) { return (result, loadCallEx) -> { if (loadCallEx == null) { - cachedValueStore.set(cacheKey, result) + valueCache.set(cacheKey, result) .whenComplete((v, setCallExIgnored) -> future.complete(result)); } else { future.completeExceptionally(loadCallEx); diff --git a/src/main/java/org/dataloader/DataLoaderOptions.java b/src/main/java/org/dataloader/DataLoaderOptions.java index 4a9d4d6..89530e1 100644 --- a/src/main/java/org/dataloader/DataLoaderOptions.java +++ b/src/main/java/org/dataloader/DataLoaderOptions.java @@ -40,7 +40,7 @@ public class DataLoaderOptions { private boolean cachingExceptionsEnabled; private CacheKey cacheKeyFunction; private CacheMap cacheMap; - private CachedValueStore cachedValueStore; + private ValueCache valueCache; private int maxBatchSize; private Supplier statisticsCollector; private BatchLoaderContextProvider environmentProvider; @@ -259,26 +259,25 @@ public DataLoaderOptions setBatchLoaderContextProvider(BatchLoaderContextProvide } /** - * Gets the (optional) cache store implementation that is used for value storage, if caching is enabled. + * Gets the (optional) cache store implementation that is used for value caching, if caching is enabled. *

* If missing, a no-op implementation will be used. * * @return an optional with the cache store instance, or empty */ - public Optional> cachedValueStore() { - return Optional.ofNullable(cachedValueStore); + public Optional> valueCache() { + return Optional.ofNullable(valueCache); } /** - * Sets the value store implementation to use for caching values, if caching is enabled. + * Sets the value cache implementation to use for caching values, if caching is enabled. * - * @param cachedValueStore the cache store instance + * @param valueCache the value cache instance * * @return the data loader options for fluent coding */ - public DataLoaderOptions setCachedValueStore(CachedValueStore cachedValueStore) { - this.cachedValueStore = cachedValueStore; + public DataLoaderOptions setValueCache(ValueCache valueCache) { + this.valueCache = valueCache; return this; } - } diff --git a/src/main/java/org/dataloader/CachedValueStore.java b/src/main/java/org/dataloader/ValueCache.java similarity index 61% rename from src/main/java/org/dataloader/CachedValueStore.java rename to src/main/java/org/dataloader/ValueCache.java index c8ed7f8..a35dc7f 100644 --- a/src/main/java/org/dataloader/CachedValueStore.java +++ b/src/main/java/org/dataloader/ValueCache.java @@ -1,16 +1,26 @@ package org.dataloader; import org.dataloader.annotations.PublicSpi; -import org.dataloader.impl.NoOpCachedValueStore; +import org.dataloader.impl.NoOpValueCache; import java.util.concurrent.CompletableFuture; /** - * Cache value store for data loaders that use caching and want a long-lived or external cache. + * The {@link ValueCache} is used by data loaders that use caching and want a long-lived or external cache + * of values. The {@link ValueCache} is used as a place to cache values when they come back from + *

+ * It differs from {@link CacheMap} which is in fact a cache of promises to values aka {@link CompletableFuture}<V> and it rather suited + * to be a wrapper of a long lived or external value cache. {@link CompletableFuture}s cant be easily placed in an external cache + * outside the JVM say, hence the need for the {@link ValueCache}. + *

+ * {@link DataLoader}s use a two stage cache strategy if caching is enabled. If the {@link CacheMap} already has the promise to a value + * that is used. If not then the {@link ValueCache} is asked for a value, if it has one then that is returned (and cached as a promise in the {@link CacheMap}. + * If there is no value then the key is queued and loaded via the {@link BatchLoader} calls. The returned values will then be stored in + * the {@link ValueCache} and the promises to those values are also stored in the {@link CacheMap}. *

* The default implementation is a no-op store which replies with the key always missing and doesn't * store any actual results. This is to avoid duplicating the stored data between the {@link CacheMap} - * and the store. + * out of the box. *

* The API signature uses completable futures because the backing implementation MAY be a remote external cache * and hence exceptions may happen in retrieving values. @@ -21,20 +31,20 @@ * @author Craig Day */ @PublicSpi -public interface CachedValueStore { +public interface ValueCache { /** - * Creates a new store, using the default no-op implementation. + * Creates a new value cache, using the default no-op implementation. * * @param the type of cache keys * @param the type of cache values * * @return the cache store */ - static CachedValueStore defaultCachedValueStore() { + static ValueCache defaultValueCache() { //noinspection unchecked - return (CachedValueStore) NoOpCachedValueStore.NOOP; + return (ValueCache) NoOpValueCache.NOOP; } /** @@ -46,7 +56,6 @@ static CachedValueStore defaultCachedValueStore() { * * @return a future containing the cached value (which maybe null) or exceptionally completed future if the key does * not exist in the cache. - * */ CompletableFuture get(K key); diff --git a/src/main/java/org/dataloader/impl/NoOpCachedValueStore.java b/src/main/java/org/dataloader/impl/NoOpValueCache.java similarity index 81% rename from src/main/java/org/dataloader/impl/NoOpCachedValueStore.java rename to src/main/java/org/dataloader/impl/NoOpValueCache.java index 0d04616..bd82f03 100644 --- a/src/main/java/org/dataloader/impl/NoOpCachedValueStore.java +++ b/src/main/java/org/dataloader/impl/NoOpValueCache.java @@ -1,13 +1,13 @@ package org.dataloader.impl; -import org.dataloader.CachedValueStore; +import org.dataloader.ValueCache; import org.dataloader.annotations.Internal; import java.util.concurrent.CompletableFuture; /** - * Implementation of {@link CachedValueStore} that does nothing. + * Implementation of {@link ValueCache} that does nothing. *

* We don't want to store values in memory twice, so when using the default store we just * say we never have the key and complete the other methods by doing nothing. @@ -18,9 +18,9 @@ * @author Craig Day */ @Internal -public class NoOpCachedValueStore implements CachedValueStore { +public class NoOpValueCache implements ValueCache { - public static NoOpCachedValueStore NOOP = new NoOpCachedValueStore<>(); + public static NoOpValueCache NOOP = new NoOpValueCache<>(); /** * {@inheritDoc} diff --git a/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java b/src/test/java/org/dataloader/DataLoaderValueCacheTest.java similarity index 89% rename from src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java rename to src/test/java/org/dataloader/DataLoaderValueCacheTest.java index 8f135ca..1c54e91 100644 --- a/src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java +++ b/src/test/java/org/dataloader/DataLoaderValueCacheTest.java @@ -2,8 +2,8 @@ import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; -import org.dataloader.fixtures.CaffeineCachedValueStore; -import org.dataloader.fixtures.CustomCachedValueStore; +import org.dataloader.fixtures.CaffeineValueCache; +import org.dataloader.fixtures.CustomValueCache; import org.junit.Test; import java.util.ArrayList; @@ -14,7 +14,6 @@ import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; -import static java.util.concurrent.CompletableFuture.completedFuture; import static org.awaitility.Awaitility.await; import static org.dataloader.DataLoaderOptions.newOptions; import static org.dataloader.fixtures.TestKit.idLoader; @@ -25,7 +24,7 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; -public class DataLoaderCachedValueStoreTest { +public class DataLoaderValueCacheTest { @Test public void test_by_default_we_have_no_value_caching() { @@ -63,9 +62,9 @@ public void test_by_default_we_have_no_value_caching() { @Test public void should_accept_a_remote_value_store_for_caching() { - CustomCachedValueStore customStore = new CustomCachedValueStore(); + CustomValueCache customStore = new CustomValueCache(); List> loadCalls = new ArrayList<>(); - DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoaderOptions options = newOptions().setValueCache(customStore); DataLoader identityLoader = idLoader(options, loadCalls); // Fetches as expected @@ -116,10 +115,10 @@ public void can_use_caffeine_for_caching() { .maximumSize(100) .build(); - CachedValueStore customStore = new CaffeineCachedValueStore(caffeineCache); + ValueCache customStore = new CaffeineValueCache(caffeineCache); List> loadCalls = new ArrayList<>(); - DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoaderOptions options = newOptions().setValueCache(customStore); DataLoader identityLoader = idLoader(options, loadCalls); // Fetches as expected @@ -147,7 +146,7 @@ public void can_use_caffeine_for_caching() { @Test public void will_invoke_loader_if_CACHE_GET_call_throws_exception() { - CustomCachedValueStore customStore = new CustomCachedValueStore() { + CustomValueCache customStore = new CustomValueCache() { @Override public CompletableFuture get(String key) { @@ -161,7 +160,7 @@ public CompletableFuture get(String key) { customStore.set("b", "From Cache"); List> loadCalls = new ArrayList<>(); - DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoaderOptions options = newOptions().setValueCache(customStore); DataLoader identityLoader = idLoader(options, loadCalls); CompletableFuture fA = identityLoader.load("a"); @@ -177,7 +176,7 @@ public CompletableFuture get(String key) { @Test public void will_still_work_if_CACHE_SET_call_throws_exception() { - CustomCachedValueStore customStore = new CustomCachedValueStore() { + CustomValueCache customStore = new CustomValueCache() { @Override public CompletableFuture set(String key, Object value) { if (key.equals("a")) { @@ -188,7 +187,7 @@ public CompletableFuture set(String key, Object value) { }; List> loadCalls = new ArrayList<>(); - DataLoaderOptions options = newOptions().setCachedValueStore(customStore); + DataLoaderOptions options = newOptions().setValueCache(customStore); DataLoader identityLoader = idLoader(options, loadCalls); CompletableFuture fA = identityLoader.load("a"); diff --git a/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java b/src/test/java/org/dataloader/fixtures/CaffeineValueCache.java similarity index 85% rename from src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java rename to src/test/java/org/dataloader/fixtures/CaffeineValueCache.java index 14520a3..2dce1a0 100644 --- a/src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java +++ b/src/test/java/org/dataloader/fixtures/CaffeineValueCache.java @@ -2,16 +2,16 @@ import com.github.benmanes.caffeine.cache.Cache; -import org.dataloader.CachedValueStore; +import org.dataloader.ValueCache; import org.dataloader.impl.CompletableFutureKit; import java.util.concurrent.CompletableFuture; -public class CaffeineCachedValueStore implements CachedValueStore { +public class CaffeineValueCache implements ValueCache { public final Cache cache; - public CaffeineCachedValueStore(Cache cache) { + public CaffeineValueCache(Cache cache) { this.cache = cache; } diff --git a/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java b/src/test/java/org/dataloader/fixtures/CustomValueCache.java similarity index 89% rename from src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java rename to src/test/java/org/dataloader/fixtures/CustomValueCache.java index e184b0c..d707175 100644 --- a/src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java +++ b/src/test/java/org/dataloader/fixtures/CustomValueCache.java @@ -1,14 +1,14 @@ package org.dataloader.fixtures; -import org.dataloader.CachedValueStore; +import org.dataloader.ValueCache; import org.dataloader.impl.CompletableFutureKit; import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; -public class CustomCachedValueStore implements CachedValueStore { +public class CustomValueCache implements ValueCache { public final Map store = new ConcurrentHashMap<>(); From 0f3dbef0f975cf1be2005927083a664372d1d12e Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 5 Jul 2021 20:53:36 +1000 Subject: [PATCH 5/5] PR feedback - bad javadoc --- src/main/java/org/dataloader/CacheMap.java | 4 ++-- src/main/java/org/dataloader/ValueCache.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/dataloader/CacheMap.java b/src/main/java/org/dataloader/CacheMap.java index d2d0408..0db31b8 100644 --- a/src/main/java/org/dataloader/CacheMap.java +++ b/src/main/java/org/dataloader/CacheMap.java @@ -22,12 +22,12 @@ import java.util.concurrent.CompletableFuture; /** - * CacheMap is used by data loaders that use caching promises to values aka {@link CompletableFuture}<V>. A better name for this + * CacheMap is used by data loaders that use caching promises to values aka {@link CompletableFuture}<V>. A better name for this * class might have been FutureCache but that is history now. *

* The default implementation used by the data loader is based on a {@link java.util.LinkedHashMap}. *

- * This is really a cache of completed {@link CompletableFuture}<V> values in memory. It is used, when caching is enabled, to + * This is really a cache of completed {@link CompletableFuture}<V> values in memory. It is used, when caching is enabled, to * give back the same future to any code that may call it. If you need a cache of the underlying values that is possible external to the JVM * then you will want to use {{@link ValueCache}} which is designed for external cache access. * diff --git a/src/main/java/org/dataloader/ValueCache.java b/src/main/java/org/dataloader/ValueCache.java index a35dc7f..31042c6 100644 --- a/src/main/java/org/dataloader/ValueCache.java +++ b/src/main/java/org/dataloader/ValueCache.java @@ -9,7 +9,7 @@ * The {@link ValueCache} is used by data loaders that use caching and want a long-lived or external cache * of values. The {@link ValueCache} is used as a place to cache values when they come back from *

- * It differs from {@link CacheMap} which is in fact a cache of promises to values aka {@link CompletableFuture}<V> and it rather suited + * It differs from {@link CacheMap} which is in fact a cache of promises to values aka {@link CompletableFuture}<V> and it rather suited * to be a wrapper of a long lived or external value cache. {@link CompletableFuture}s cant be easily placed in an external cache * outside the JVM say, hence the need for the {@link ValueCache}. *