From 98d46b55344fef87409eb981b905aa877c3b1e21 Mon Sep 17 00:00:00 2001 From: Craig Day Date: Wed, 5 May 2021 12:14:52 -0700 Subject: [PATCH 1/2] cache values and errors, not futures --- src/main/java/org/dataloader/CacheMap.java | 42 +++++++++++--- src/main/java/org/dataloader/DataLoader.java | 24 ++++---- .../java/org/dataloader/DataLoaderHelper.java | 49 ++++++++++++---- .../org/dataloader/impl/DefaultCacheMap.java | 58 ++++++++++++++++--- src/test/java/ReadmeExamples.java | 17 +++++- .../java/org/dataloader/CustomCacheMap.java | 22 ++++++- 6 files changed, 170 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/dataloader/CacheMap.java b/src/main/java/org/dataloader/CacheMap.java index f60c6ef..894d60d 100644 --- a/src/main/java/org/dataloader/CacheMap.java +++ b/src/main/java/org/dataloader/CacheMap.java @@ -18,12 +18,10 @@ import org.dataloader.impl.DefaultCacheMap; -import java.util.concurrent.CompletableFuture; - /** * Cache map interface for data loaders that use caching. *

- * The default implementation used by the data loader is based on a {@link java.util.LinkedHashMap}. Note that the + * The default implementation used by the data loader is based on a {@link java.util.concurrent.ConcurrentHashMap}. 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 *

@@ -39,19 +37,19 @@ public interface CacheMap { /** - * Creates a new cache map, using the default implementation that is based on a {@link java.util.LinkedHashMap}. + * Creates a new cache map, using the default implementation that is based on a {@link java.util.concurrent.ConcurrentHashMap}. * * @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 * @@ -69,7 +67,7 @@ static CacheMap> simpleMap() { * * @return the cached value, or {@code null} if not found (depends on cache implementation) */ - V get(U key); + Try get(U key); /** * Creates a new cache map entry with the specified key and value, or updates the value if the key already exists. @@ -81,6 +79,36 @@ static CacheMap> simpleMap() { */ CacheMap set(U key, V value); + /** + * Creates a new cache entry with the specified key and error, or updates the error if the key already exists. + * + * @param key the key to cache + * @param error the error to cache + * + * @return the cache map for fluent coding + */ + CacheMap set(U key, Throwable error); + + /** + * Creates a new cache map entry with the specified key and value if it doesn't exist. + * + * @param key the key to cache + * @param value the value to cache + * + * @return the cache map for fluent coding + */ + CacheMap setIfAbsent(U key, V value); + + /** + * Creates a new cache map entry with the specified key and error if it doesn't exist. + * + * @param key the key to cache + * @param error the value to cache + * + * @return the cache map for fluent coding + */ + CacheMap setIfAbsent(U key, Throwable error); + /** * Deletes the entry with the specified key from the cache map, if it exists. * diff --git a/src/main/java/org/dataloader/DataLoader.java b/src/main/java/org/dataloader/DataLoader.java index 3f200c3..d82a23e 100644 --- a/src/main/java/org/dataloader/DataLoader.java +++ b/src/main/java/org/dataloader/DataLoader.java @@ -58,7 +58,7 @@ public class DataLoader { private final DataLoaderHelper helper; - private final CacheMap> futureCache; + private final CacheMap valueCache; private final StatisticsCollector stats; /** @@ -332,16 +332,16 @@ public DataLoader(BatchLoader batchLoadFunction, DataLoaderOptions options private DataLoader(Object batchLoadFunction, DataLoaderOptions options) { DataLoaderOptions loaderOptions = options == null ? new DataLoaderOptions() : options; - this.futureCache = determineCacheMap(loaderOptions); + this.valueCache = determineCacheMap(loaderOptions); // order of keys matter in data loader this.stats = nonNull(loaderOptions.getStatisticsCollector()); - this.helper = new DataLoaderHelper<>(this, batchLoadFunction, loaderOptions, this.futureCache, this.stats); + this.helper = new DataLoaderHelper<>(this, batchLoadFunction, loaderOptions, this.valueCache, this.stats); } @SuppressWarnings("unchecked") - private CacheMap> determineCacheMap(DataLoaderOptions loaderOptions) { - return loaderOptions.cacheMap().isPresent() ? (CacheMap>) loaderOptions.cacheMap().get() : CacheMap.simpleMap(); + private CacheMap determineCacheMap(DataLoaderOptions loaderOptions) { + return loaderOptions.cacheMap().isPresent() ? (CacheMap) loaderOptions.cacheMap().get() : CacheMap.simpleMap(); } /** @@ -521,7 +521,7 @@ public int dispatchDepth() { public DataLoader clear(K key) { Object cacheKey = getCacheKey(key); synchronized (this) { - futureCache.delete(cacheKey); + valueCache.delete(cacheKey); } return this; } @@ -533,7 +533,7 @@ public DataLoader clear(K key) { */ public DataLoader clearAll() { synchronized (this) { - futureCache.clear(); + valueCache.clear(); } return this; } @@ -548,9 +548,7 @@ public DataLoader clearAll() { public DataLoader prime(K key, V value) { Object cacheKey = getCacheKey(key); synchronized (this) { - if (!futureCache.containsKey(cacheKey)) { - futureCache.set(cacheKey, CompletableFuture.completedFuture(value)); - } + valueCache.setIfAbsent(cacheKey, value); } return this; } @@ -562,10 +560,10 @@ public DataLoader prime(K key, V value) { * @param error the exception to prime instead of a value * @return the data loader for fluent coding */ - public DataLoader prime(K key, Exception error) { + public DataLoader prime(K key, Throwable error) { Object cacheKey = getCacheKey(key); - if (!futureCache.containsKey(cacheKey)) { - futureCache.set(cacheKey, CompletableFutureKit.failedFuture(error)); + synchronized (this) { + valueCache.setIfAbsent(cacheKey, error); } return this; } diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java index edef507..9c85c4c 100644 --- a/src/main/java/org/dataloader/DataLoaderHelper.java +++ b/src/main/java/org/dataloader/DataLoaderHelper.java @@ -1,5 +1,6 @@ package org.dataloader; +import java.util.concurrent.atomic.AtomicReference; import org.dataloader.impl.CompletableFutureKit; import org.dataloader.stats.StatisticsCollector; @@ -58,15 +59,15 @@ Object getCallContext() { private final DataLoader dataLoader; private final Object batchLoadFunction; private final DataLoaderOptions loaderOptions; - private final CacheMap> futureCache; + private final CacheMap valueCache; private final List>> loaderQueue; private final StatisticsCollector stats; - DataLoaderHelper(DataLoader dataLoader, Object batchLoadFunction, DataLoaderOptions loaderOptions, CacheMap> futureCache, StatisticsCollector stats) { + DataLoaderHelper(DataLoader dataLoader, Object batchLoadFunction, DataLoaderOptions loaderOptions, CacheMap valueCache, StatisticsCollector stats) { this.dataLoader = dataLoader; this.batchLoadFunction = batchLoadFunction; this.loaderOptions = loaderOptions; - this.futureCache = futureCache; + this.valueCache = valueCache; this.loaderQueue = new ArrayList<>(); this.stats = stats; } @@ -76,9 +77,19 @@ Optional> getIfPresent(K key) { boolean cachingEnabled = loaderOptions.cachingEnabled(); if (cachingEnabled) { Object cacheKey = getCacheKey(nonNull(key)); - if (futureCache.containsKey(cacheKey)) { + if (valueCache.containsKey(cacheKey)) { stats.incrementCacheHitCount(); - return Optional.of(futureCache.get(cacheKey)); + + CompletableFuture futureValue = new CompletableFuture<>(); + Try cacheResult = valueCache.get(cacheKey); + + if (cacheResult.isFailure()) { + futureValue.completeExceptionally(cacheResult.getThrowable()); + } else { + futureValue.complete(cacheResult.get()); + } + + return Optional.of(futureValue); } } } @@ -104,20 +115,30 @@ CompletableFuture load(K key, Object loadContext) { boolean batchingEnabled = loaderOptions.batchingEnabled(); boolean cachingEnabled = loaderOptions.cachingEnabled(); - Object cacheKey = null; + final AtomicReference cacheKey = new AtomicReference<>(null); if (cachingEnabled) { if (loadContext == null) { - cacheKey = getCacheKey(key); + cacheKey.set(getCacheKey(key)); } else { - cacheKey = getCacheKeyWithContext(key, loadContext); + cacheKey.set(getCacheKeyWithContext(key, loadContext)); } } stats.incrementLoadCount(); if (cachingEnabled) { - if (futureCache.containsKey(cacheKey)) { + if (valueCache.containsKey(cacheKey.get())) { stats.incrementCacheHitCount(); - return futureCache.get(cacheKey); + + CompletableFuture futureValue = new CompletableFuture<>(); + Try cacheResult = valueCache.get(cacheKey.get()); + + if (cacheResult.isFailure()) { + futureValue.completeExceptionally(cacheResult.getThrowable()); + } else { + futureValue.complete(cacheResult.get()); + } + + return futureValue; } } @@ -130,7 +151,13 @@ CompletableFuture load(K key, Object loadContext) { future = invokeLoaderImmediately(key, loadContext); } if (cachingEnabled) { - futureCache.set(cacheKey, future); + future.whenComplete((value, error) -> { + if (error != null) { + valueCache.set(cacheKey.get(), error); + } else { + valueCache.set(cacheKey.get(), value); + } + }); } return future; } diff --git a/src/main/java/org/dataloader/impl/DefaultCacheMap.java b/src/main/java/org/dataloader/impl/DefaultCacheMap.java index 0dc377e..94f6a44 100644 --- a/src/main/java/org/dataloader/impl/DefaultCacheMap.java +++ b/src/main/java/org/dataloader/impl/DefaultCacheMap.java @@ -16,14 +16,14 @@ package org.dataloader.impl; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import org.dataloader.CacheMap; import org.dataloader.Internal; - -import java.util.HashMap; -import java.util.Map; +import org.dataloader.Try; /** - * 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 {@link ConcurrentHashMap}. * * @param type parameter indicating the type of the cache keys * @param type parameter indicating the type of the data that is cached @@ -34,12 +34,14 @@ public class DefaultCacheMap implements CacheMap { private final Map cache; + private final Map errorCache; /** * Default constructor */ public DefaultCacheMap() { - cache = new HashMap<>(); + cache = new ConcurrentHashMap<>(); + errorCache = new ConcurrentHashMap<>(); } /** @@ -47,15 +49,22 @@ public DefaultCacheMap() { */ @Override public boolean containsKey(U key) { - return cache.containsKey(key); + return cache.containsKey(key) || errorCache.containsKey(key); } /** * {@inheritDoc} */ @Override - public V get(U key) { - return cache.get(key); + public Try get(U key) { + final Throwable error = errorCache.get(key); + final V value = cache.get(key); + + if (error != null) { + return Try.failed(error); + } else { + return Try.succeeded(value); + } } /** @@ -64,15 +73,47 @@ public V get(U key) { @Override public CacheMap set(U key, V value) { cache.put(key, value); + errorCache.remove(key); + return this; + } + + /** + * {@inheritDoc} + */ + @Override + public CacheMap set(U key, Throwable error) { + cache.remove(key); + errorCache.put(key, error); + return this; + } + + /** + * {@inheritDoc} + */ + @Override + public CacheMap setIfAbsent(U key, V value) { + cache.putIfAbsent(key, value); + errorCache.remove(key); return this; } + /** + * {@inheritDoc} + */ + @Override + public CacheMap setIfAbsent(U key, Throwable error) { + cache.remove(key); + errorCache.putIfAbsent(key, error); + return null; + } + /** * {@inheritDoc} */ @Override public CacheMap delete(U key) { cache.remove(key); + errorCache.remove(key); return this; } @@ -82,6 +123,7 @@ public CacheMap delete(U key) { @Override public CacheMap clear() { cache.clear(); + errorCache.clear(); return this; } } diff --git a/src/test/java/ReadmeExamples.java b/src/test/java/ReadmeExamples.java index 523cb5a..dd49e73 100644 --- a/src/test/java/ReadmeExamples.java +++ b/src/test/java/ReadmeExamples.java @@ -213,7 +213,7 @@ public boolean containsKey(Object key) { } @Override - public Object get(Object key) { + public Try get(Object key) { return null; } @@ -222,6 +222,21 @@ public CacheMap set(Object key, Object value) { return null; } + @Override + public CacheMap set(Object key, Throwable error) { + return null; + } + + @Override + public CacheMap setIfAbsent(Object key, Object value) { + return null; + } + + @Override + public CacheMap setIfAbsent(Object key, Throwable error) { + return null; + } + @Override public CacheMap delete(Object key) { return null; diff --git a/src/test/java/org/dataloader/CustomCacheMap.java b/src/test/java/org/dataloader/CustomCacheMap.java index 505148d..4c6e4e6 100644 --- a/src/test/java/org/dataloader/CustomCacheMap.java +++ b/src/test/java/org/dataloader/CustomCacheMap.java @@ -17,8 +17,8 @@ public boolean containsKey(String key) { } @Override - public Object get(String key) { - return stash.get(key); + public Try get(String key) { + return Try.succeeded(stash.get(key)); } @Override @@ -27,6 +27,24 @@ public CacheMap set(String key, Object value) { return this; } + @Override + public CacheMap set(String key, Throwable error) { + // Don't cache errors in this implementation + return this; + } + + @Override + public CacheMap setIfAbsent(String key, Object value) { + stash.putIfAbsent(key, value); + return this; + } + + @Override + public CacheMap setIfAbsent(String key, Throwable error) { + // Don't cache errors in this implementation + return this; + } + @Override public CacheMap delete(String key) { stash.remove(key); From d5bfdd52f47d815f679e07631daba78ff32bd75d Mon Sep 17 00:00:00 2001 From: Craig Day Date: Wed, 5 May 2021 18:46:54 -0700 Subject: [PATCH 2/2] drop setIfAbsent to stay close to reference; update docs; helper fn --- src/main/java/org/dataloader/CacheMap.java | 26 +++---------------- src/main/java/org/dataloader/DataLoader.java | 8 ++++-- .../java/org/dataloader/DataLoaderHelper.java | 24 ++--------------- .../dataloader/impl/CompletableFutureKit.java | 11 +++++++- .../org/dataloader/impl/DefaultCacheMap.java | 20 -------------- src/test/java/ReadmeExamples.java | 10 ------- .../java/org/dataloader/CustomCacheMap.java | 12 --------- 7 files changed, 22 insertions(+), 89 deletions(-) diff --git a/src/main/java/org/dataloader/CacheMap.java b/src/main/java/org/dataloader/CacheMap.java index 894d60d..caa1259 100644 --- a/src/main/java/org/dataloader/CacheMap.java +++ b/src/main/java/org/dataloader/CacheMap.java @@ -23,7 +23,7 @@ *

* The default implementation used by the data loader is based on a {@link java.util.concurrent.ConcurrentHashMap}. 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 + * 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. * @@ -60,12 +60,14 @@ static CacheMap simpleMap() { /** * Gets the specified key from the cache map. *

+ * The result is wrapped in a {@link Try} to support caching both values and errors. + *

* May throw an exception if the key does not exists, depending on the cache map implementation that is used, * so be sure to check {@link CacheMap#containsKey(Object)} first. * * @param key the key to retrieve * - * @return the cached value, or {@code null} if not found (depends on cache implementation) + * @return the cached value, error, or {@code null} if not found (depends on cache implementation) */ Try get(U key); @@ -89,26 +91,6 @@ static CacheMap simpleMap() { */ CacheMap set(U key, Throwable error); - /** - * Creates a new cache map entry with the specified key and value if it doesn't exist. - * - * @param key the key to cache - * @param value the value to cache - * - * @return the cache map for fluent coding - */ - CacheMap setIfAbsent(U key, V value); - - /** - * Creates a new cache map entry with the specified key and error if it doesn't exist. - * - * @param key the key to cache - * @param error the value to cache - * - * @return the cache map for fluent coding - */ - CacheMap setIfAbsent(U key, Throwable error); - /** * Deletes the entry with the specified key from the cache map, if it exists. * diff --git a/src/main/java/org/dataloader/DataLoader.java b/src/main/java/org/dataloader/DataLoader.java index d82a23e..1c10050 100644 --- a/src/main/java/org/dataloader/DataLoader.java +++ b/src/main/java/org/dataloader/DataLoader.java @@ -548,7 +548,9 @@ public DataLoader clearAll() { public DataLoader prime(K key, V value) { Object cacheKey = getCacheKey(key); synchronized (this) { - valueCache.setIfAbsent(cacheKey, value); + if (!valueCache.containsKey(cacheKey)) { + valueCache.set(cacheKey, value); + } } return this; } @@ -563,7 +565,9 @@ public DataLoader prime(K key, V value) { public DataLoader prime(K key, Throwable error) { Object cacheKey = getCacheKey(key); synchronized (this) { - valueCache.setIfAbsent(cacheKey, error); + if (!valueCache.containsKey(cacheKey)) { + valueCache.set(cacheKey, error); + } } return this; } diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java index 9c85c4c..8e57abe 100644 --- a/src/main/java/org/dataloader/DataLoaderHelper.java +++ b/src/main/java/org/dataloader/DataLoaderHelper.java @@ -79,17 +79,7 @@ Optional> getIfPresent(K key) { Object cacheKey = getCacheKey(nonNull(key)); if (valueCache.containsKey(cacheKey)) { stats.incrementCacheHitCount(); - - CompletableFuture futureValue = new CompletableFuture<>(); - Try cacheResult = valueCache.get(cacheKey); - - if (cacheResult.isFailure()) { - futureValue.completeExceptionally(cacheResult.getThrowable()); - } else { - futureValue.complete(cacheResult.get()); - } - - return Optional.of(futureValue); + return Optional.of(CompletableFutureKit.fromTry(valueCache.get(key))); } } } @@ -128,17 +118,7 @@ CompletableFuture load(K key, Object loadContext) { if (cachingEnabled) { if (valueCache.containsKey(cacheKey.get())) { stats.incrementCacheHitCount(); - - CompletableFuture futureValue = new CompletableFuture<>(); - Try cacheResult = valueCache.get(cacheKey.get()); - - if (cacheResult.isFailure()) { - futureValue.completeExceptionally(cacheResult.getThrowable()); - } else { - futureValue.complete(cacheResult.get()); - } - - return futureValue; + return CompletableFutureKit.fromTry(valueCache.get(cacheKey.get())); } } diff --git a/src/main/java/org/dataloader/impl/CompletableFutureKit.java b/src/main/java/org/dataloader/impl/CompletableFutureKit.java index 3cce6b5..9445977 100644 --- a/src/main/java/org/dataloader/impl/CompletableFutureKit.java +++ b/src/main/java/org/dataloader/impl/CompletableFutureKit.java @@ -5,6 +5,7 @@ import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; +import org.dataloader.Try; import static java.util.stream.Collectors.toList; @@ -14,7 +15,7 @@ @Internal public class CompletableFutureKit { - public static CompletableFuture failedFuture(Exception e) { + public static CompletableFuture failedFuture(Throwable e) { CompletableFuture future = new CompletableFuture<>(); future.completeExceptionally(e); return future; @@ -54,4 +55,12 @@ public static CompletableFuture> allOf(List> cf .collect(toList()) ); } + + public static CompletableFuture fromTry(Try tryResult) { + if (tryResult.isFailure()) { + return failedFuture(tryResult.getThrowable()); + } else { + return CompletableFuture.completedFuture(tryResult.get()); + } + } } diff --git a/src/main/java/org/dataloader/impl/DefaultCacheMap.java b/src/main/java/org/dataloader/impl/DefaultCacheMap.java index 94f6a44..e382074 100644 --- a/src/main/java/org/dataloader/impl/DefaultCacheMap.java +++ b/src/main/java/org/dataloader/impl/DefaultCacheMap.java @@ -87,26 +87,6 @@ public CacheMap set(U key, Throwable error) { return this; } - /** - * {@inheritDoc} - */ - @Override - public CacheMap setIfAbsent(U key, V value) { - cache.putIfAbsent(key, value); - errorCache.remove(key); - return this; - } - - /** - * {@inheritDoc} - */ - @Override - public CacheMap setIfAbsent(U key, Throwable error) { - cache.remove(key); - errorCache.putIfAbsent(key, error); - return null; - } - /** * {@inheritDoc} */ diff --git a/src/test/java/ReadmeExamples.java b/src/test/java/ReadmeExamples.java index dd49e73..5d63e5e 100644 --- a/src/test/java/ReadmeExamples.java +++ b/src/test/java/ReadmeExamples.java @@ -227,16 +227,6 @@ public CacheMap set(Object key, Throwable error) { return null; } - @Override - public CacheMap setIfAbsent(Object key, Object value) { - return null; - } - - @Override - public CacheMap setIfAbsent(Object key, Throwable error) { - return null; - } - @Override public CacheMap delete(Object key) { return null; diff --git a/src/test/java/org/dataloader/CustomCacheMap.java b/src/test/java/org/dataloader/CustomCacheMap.java index 4c6e4e6..0700384 100644 --- a/src/test/java/org/dataloader/CustomCacheMap.java +++ b/src/test/java/org/dataloader/CustomCacheMap.java @@ -33,18 +33,6 @@ public CacheMap set(String key, Throwable error) { return this; } - @Override - public CacheMap setIfAbsent(String key, Object value) { - stash.putIfAbsent(key, value); - return this; - } - - @Override - public CacheMap setIfAbsent(String key, Throwable error) { - // Don't cache errors in this implementation - return this; - } - @Override public CacheMap delete(String key) { stash.remove(key);