PR feedback - name of ValueCache

Author: bbakerman

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.
 
 

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.
  * <p>
- * 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}.
  * <p>
- * 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}&lt;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 <K> type parameter indicating the type of the cache keys
  * @param <V> type parameter indicating the type of the data that is cached

src/main/java/org/dataloader/DataLoader.java

@@ -67,7 +67,7 @@ public class DataLoader<K, V> {
     private final DataLoaderHelper<K, V> helper;
     private final StatisticsCollector stats;
     private final CacheMap<Object, V> futureCache;
-    private final CachedValueStore<Object, V> cachedValueStore;
+    private final ValueCache<Object, V> valueCache;
 
     /**
      * Creates new DataLoader with the specified batch loader function and default options
@@ -416,11 +416,11 @@ public DataLoader(BatchLoader<K, V> 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<Object, V> determineFutureCache(DataLoaderOptions loaderOptions
     }
 
     @SuppressWarnings("unchecked")
-    private CachedValueStore<Object, V> determineCachedValueStore(DataLoaderOptions loaderOptions) {
-        return (CachedValueStore<Object, V>) loaderOptions.cachedValueStore().orElseGet(CachedValueStore::defaultCachedValueStore);
+    private ValueCache<Object, V> determineValueCache(DataLoaderOptions loaderOptions) {
+        return (ValueCache<Object, V>) loaderOptions.valueCache().orElseGet(ValueCache::defaultValueCache);
     }
 
     /**
@@ -652,7 +652,7 @@ public DataLoader<K, V> clear(K key, BiConsumer<Void, Throwable> 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<K, V> clearAll() {
     public DataLoader<K, V> clearAll(BiConsumer<Void, Throwable> 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

src/main/java/org/dataloader/DataLoaderHelper.java

@@ -63,7 +63,7 @@ Object getCallContext() {
     private final Object batchLoadFunction;
     private final DataLoaderOptions loaderOptions;
     private final CacheMap<Object, V> futureCache;
-    private final CachedValueStore<Object, V> cachedValueStore;
+    private final ValueCache<Object, V> valueCache;
     private final List<LoaderQueueEntry<K, CompletableFuture<V>>> loaderQueue;
     private final StatisticsCollector stats;
     private final Clock clock;
@@ -73,14 +73,14 @@ Object getCallContext() {
                      Object batchLoadFunction,
                      DataLoaderOptions loaderOptions,
                      CacheMap<Object, V> futureCache,
-                     CachedValueStore<Object, V> cachedValueStore,
+                     ValueCache<Object, V> 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<V> loadFromCache(K key, Object loadContext, boolean ba
          */
         final CompletableFuture<V> 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<V> loadFromCache(K key, Object loadContext, boolean ba
     private BiConsumer<V, Throwable> setValueIntoCacheAndCompleteFuture(Object cacheKey, CompletableFuture<V> 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);

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> 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.
      * <p>
      * If missing, a no-op implementation will be used.
      *
      * @return an optional with the cache store instance, or empty
      */
-    public Optional<CachedValueStore<?,?>> cachedValueStore() {
-        return Optional.ofNullable(cachedValueStore);
+    public Optional<ValueCache<?,?>> 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;
     }
-
 }

src/main/java/org/dataloader/CachedValueStore.java renamed to 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
+ * <p>
+ * It differs from {@link CacheMap} which is in fact a cache of promises to values aka {@link CompletableFuture}&lt;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}.
+ * <p>
+ * {@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}.
  * <p>
  * 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.
  * <p>
  * 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 <a href="https://github.com/craig-day">Craig Day</a>
  */
 @PublicSpi
-public interface CachedValueStore<K, V> {
+public interface ValueCache<K, V> {
 
 
     /**
-     * Creates a new store, using the default no-op implementation.
+     * Creates a new value cache, using the default no-op implementation.
      *
      * @param <K> the type of cache keys
      * @param <V> the type of cache values
      *
      * @return the cache store
      */
-    static <K, V> CachedValueStore<K, V> defaultCachedValueStore() {
+    static <K, V> ValueCache<K, V> defaultValueCache() {
         //noinspection unchecked
-        return (CachedValueStore<K, V>) NoOpCachedValueStore.NOOP;
+        return (ValueCache<K, V>) NoOpValueCache.NOOP;
     }
 
     /**
@@ -46,7 +56,6 @@ static <K, V> CachedValueStore<K, V> 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<V> get(K key);
 

src/main/java/org/dataloader/impl/NoOpCachedValueStore.java renamed to 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.
  * <p>
  * 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 <a href="https://github.com/craig-day">Craig Day</a>
  */
 @Internal
-public class NoOpCachedValueStore<K, V> implements CachedValueStore<K, V> {
+public class NoOpValueCache<K, V> implements ValueCache<K, V> {
 
-    public static NoOpCachedValueStore<?, ?> NOOP = new NoOpCachedValueStore<>();
+    public static NoOpValueCache<?, ?> NOOP = new NoOpValueCache<>();
 
     /**
      * {@inheritDoc}

src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java renamed to 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<List<String>> loadCalls = new ArrayList<>();
-        DataLoaderOptions options = newOptions().setCachedValueStore(customStore);
+        DataLoaderOptions options = newOptions().setValueCache(customStore);
         DataLoader<String, String> identityLoader = idLoader(options, loadCalls);
 
         // Fetches as expected
@@ -116,10 +115,10 @@ public void can_use_caffeine_for_caching() {
                 .maximumSize(100)
                 .build();
 
-        CachedValueStore<String, Object> customStore = new CaffeineCachedValueStore(caffeineCache);
+        ValueCache<String, Object> customStore = new CaffeineValueCache(caffeineCache);
 
         List<List<String>> loadCalls = new ArrayList<>();
-        DataLoaderOptions options = newOptions().setCachedValueStore(customStore);
+        DataLoaderOptions options = newOptions().setValueCache(customStore);
         DataLoader<String, String> 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<Object> get(String key) {
@@ -161,7 +160,7 @@ public CompletableFuture<Object> get(String key) {
         customStore.set("b", "From Cache");
 
         List<List<String>> loadCalls = new ArrayList<>();
-        DataLoaderOptions options = newOptions().setCachedValueStore(customStore);
+        DataLoaderOptions options = newOptions().setValueCache(customStore);
         DataLoader<String, String> identityLoader = idLoader(options, loadCalls);
 
         CompletableFuture<String> fA = identityLoader.load("a");
@@ -177,7 +176,7 @@ public CompletableFuture<Object> 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<Object> set(String key, Object value) {
                 if (key.equals("a")) {
@@ -188,7 +187,7 @@ public CompletableFuture<Object> set(String key, Object value) {
         };
 
         List<List<String>> loadCalls = new ArrayList<>();
-        DataLoaderOptions options = newOptions().setCachedValueStore(customStore);
+        DataLoaderOptions options = newOptions().setValueCache(customStore);
         DataLoader<String, String> identityLoader = idLoader(options, loadCalls);
 
         CompletableFuture<String> fA = identityLoader.load("a");