From 747a7d03ee48ba692e1090d99be171e6d5123641 Mon Sep 17 00:00:00 2001 From: John Blum Date: Thu, 10 Aug 2023 17:31:20 -0700 Subject: [PATCH] Refactor code and cleanup compiler warnings in Redis caching infrastructure. * Apply Java 17 syntax try-with-resources in DefaultRedisCacheWriter execute methods. * Organize source code * Edit Javadoc. Closes #2733 --- .../data/redis/cache/BatchStrategies.java | 22 ++-- .../redis/cache/DefaultRedisCacheWriter.java | 102 +++++++++--------- .../data/redis/cache/RedisCache.java | 31 ++++-- .../data/redis/cache/RedisCacheManager.java | 38 +++++-- 4 files changed, 112 insertions(+), 81 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/cache/BatchStrategies.java b/src/main/java/org/springframework/data/redis/cache/BatchStrategies.java index f77b454715..fa4c78e865 100644 --- a/src/main/java/org/springframework/data/redis/cache/BatchStrategies.java +++ b/src/main/java/org/springframework/data/redis/cache/BatchStrategies.java @@ -28,18 +28,15 @@ import org.springframework.util.Assert; /** - * A collection of predefined {@link BatchStrategy} implementations using {@code KEYS} or {@code SCAN} command. + * Collection of predefined {@link BatchStrategy} implementations using the Redis {@code KEYS} or {@code SCAN} command. * * @author Mark Paluch * @author Christoph Strobl + * @author John Blum * @since 2.6 */ public abstract class BatchStrategies { - private BatchStrategies() { - // can't touch this - oh-oh oh oh oh-oh-oh - } - /** * A {@link BatchStrategy} using a single {@code KEYS} and {@code DEL} command to remove all matching keys. * {@code KEYS} scans the entire keyspace of the Redis database and can block the Redis worker thread for a long time @@ -68,6 +65,10 @@ public static BatchStrategy scan(int batchSize) { return new Scan(batchSize); } + private BatchStrategies() { + // can't touch this - oh-oh oh oh oh-oh-oh + } + /** * {@link BatchStrategy} using {@code KEYS}. */ @@ -108,9 +109,11 @@ public long cleanCache(RedisConnection connection, String name, byte[] pattern) long count = 0; PartitionIterator partitions = new PartitionIterator<>(cursor, batchSize); + while (partitions.hasNext()) { List keys = partitions.next(); + count += keys.size(); if (keys.size() > 0) { @@ -141,7 +144,7 @@ static class PartitionIterator implements Iterator> { @Override public boolean hasNext() { - return iterator.hasNext(); + return this.iterator.hasNext(); } @Override @@ -151,9 +154,10 @@ public List next() { throw new NoSuchElementException(); } - List list = new ArrayList<>(size); - while (list.size() < size && iterator.hasNext()) { - list.add(iterator.next()); + List list = new ArrayList<>(this.size); + + while (list.size() < this.size && this.iterator.hasNext()) { + list.add(this.iterator.next()); } return list; diff --git a/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java b/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java index 36121249d6..0b699ab9a9 100644 --- a/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java +++ b/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java @@ -102,27 +102,6 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { this.batchStrategy = batchStrategy; } - @Override - public void put(String name, byte[] key, byte[] value, @Nullable Duration ttl) { - - Assert.notNull(name, "Name must not be null"); - Assert.notNull(key, "Key must not be null"); - Assert.notNull(value, "Value must not be null"); - - execute(name, connection -> { - - if (shouldExpireWithin(ttl)) { - connection.set(key, value, Expiration.from(ttl.toMillis(), TimeUnit.MILLISECONDS), SetOption.upsert()); - } else { - connection.set(key, value); - } - - return "OK"; - }); - - statistics.incPuts(name); - } - @Override public byte[] get(String name, byte[] key) { return get(name, key, null); @@ -135,8 +114,8 @@ public byte[] get(String name, byte[] key, @Nullable Duration ttl) { Assert.notNull(key, "Key must not be null"); byte[] result = shouldExpireWithin(ttl) - ? execute(name, connection -> connection.getEx(key, Expiration.from(ttl))) - : execute(name, connection -> connection.get(key)); + ? execute(name, connection -> connection.stringCommands().getEx(key, Expiration.from(ttl))) + : execute(name, connection -> connection.stringCommands().get(key)); statistics.incGets(name); @@ -149,6 +128,28 @@ public byte[] get(String name, byte[] key, @Nullable Duration ttl) { return result; } + @Override + public void put(String name, byte[] key, byte[] value, @Nullable Duration ttl) { + + Assert.notNull(name, "Name must not be null"); + Assert.notNull(key, "Key must not be null"); + Assert.notNull(value, "Value must not be null"); + + execute(name, connection -> { + + if (shouldExpireWithin(ttl)) { + connection.stringCommands() + .set(key, value, Expiration.from(ttl.toMillis(), TimeUnit.MILLISECONDS), SetOption.upsert()); + } else { + connection.stringCommands().set(key, value); + } + + return "OK"; + }); + + statistics.incPuts(name); + } + @Override public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Duration ttl) { @@ -167,9 +168,9 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Durat boolean put; if (shouldExpireWithin(ttl)) { - put = connection.set(key, value, Expiration.from(ttl), SetOption.ifAbsent()); + put = isTrue(connection.stringCommands().set(key, value, Expiration.from(ttl), SetOption.ifAbsent())); } else { - put = connection.setNX(key, value); + put = isTrue(connection.stringCommands().setNX(key, value)); } if (put) { @@ -177,7 +178,7 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Durat return null; } - return connection.get(key); + return connection.stringCommands().get(key); } finally { if (isLockingCacheWriter()) { @@ -193,7 +194,7 @@ public void remove(String name, byte[] key) { Assert.notNull(name, "Name must not be null"); Assert.notNull(key, "Key must not be null"); - execute(name, connection -> connection.del(key)); + execute(name, connection -> connection.keyCommands().del(key)); statistics.incDeletes(name); } @@ -257,6 +258,16 @@ void lock(String name) { execute(name, connection -> doLock(name, name, null, connection)); } + @Nullable + private Boolean doLock(String name, Object contextualKey, @Nullable Object contextualValue, + RedisConnection connection) { + + Expiration expiration = Expiration.from(this.lockTtl.getTimeToLive(contextualKey, contextualValue)); + + return connection.stringCommands() + .set(createCacheLockKey(name), new byte[0], expiration, SetOption.SET_IF_ABSENT); + } + /** * Explicitly remove a write lock from a cache. * @@ -266,20 +277,13 @@ void unlock(String name) { executeLockFree(connection -> doUnlock(name, connection)); } - private Boolean doLock(String name, Object contextualKey, Object contextualValue, RedisConnection connection) { - - Expiration expiration = lockTtl == null ? Expiration.persistent() - : Expiration.from(lockTtl.getTimeToLive(contextualKey, contextualValue)); - - return connection.set(createCacheLockKey(name), new byte[0], expiration, SetOption.SET_IF_ABSENT); - } - + @Nullable private Long doUnlock(String name, RedisConnection connection) { - return connection.del(createCacheLockKey(name)); + return connection.keyCommands().del(createCacheLockKey(name)); } boolean doCheckLock(String name, RedisConnection connection) { - return connection.exists(createCacheLockKey(name)); + return isTrue(connection.keyCommands().exists(createCacheLockKey(name))); } /** @@ -291,24 +295,16 @@ private boolean isLockingCacheWriter() { private T execute(String name, Function callback) { - RedisConnection connection = connectionFactory.getConnection(); - - try { + try (RedisConnection connection = connectionFactory.getConnection()) { checkAndPotentiallyWaitUntilUnlocked(name, connection); return callback.apply(connection); - } finally { - connection.close(); } } private void executeLockFree(Consumer callback) { - RedisConnection connection = connectionFactory.getConnection(); - - try { + try (RedisConnection connection = connectionFactory.getConnection()) { callback.accept(connection); - } finally { - connection.close(); } } @@ -337,11 +333,15 @@ private void checkAndPotentiallyWaitUntilUnlocked(String name, RedisConnection c } } - private static boolean shouldExpireWithin(@Nullable Duration ttl) { - return ttl != null && !ttl.isZero() && !ttl.isNegative(); - } - private static byte[] createCacheLockKey(String name) { return (name + "~lock").getBytes(StandardCharsets.UTF_8); } + + private boolean isTrue(@Nullable Boolean value) { + return Boolean.TRUE.equals(value); + } + + private static boolean shouldExpireWithin(@Nullable Duration ttl) { + return ttl != null && !ttl.isZero() && !ttl.isNegative(); + } } diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCache.java b/src/main/java/org/springframework/data/redis/cache/RedisCache.java index bc17d7b7be..50776f0f93 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCache.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCache.java @@ -24,8 +24,10 @@ import java.util.Map.Entry; import java.util.StringJoiner; import java.util.concurrent.Callable; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; +import java.util.function.Supplier; import org.springframework.cache.Cache; import org.springframework.cache.support.AbstractValueAdaptingCache; @@ -37,6 +39,7 @@ import org.springframework.data.redis.serializer.RedisSerializationContext; import org.springframework.data.redis.serializer.RedisSerializer; import org.springframework.data.redis.util.ByteUtils; +import org.springframework.data.redis.util.RedisAssertions; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; @@ -69,7 +72,8 @@ public class RedisCache extends AbstractValueAdaptingCache { private final String name; /** - * Create a new {@link RedisCache} with the given {@link String name}. + * Create a new {@link RedisCache} with the given {@link String name} and {@link RedisCacheConfiguration}, + * using the {@link RedisCacheWriter} to execute Redis commands supporting the cache operations. * * @param name {@link String name} for this {@link Cache}; must not be {@literal null}. * @param cacheWriter {@link RedisCacheWriter} used to perform {@link RedisCache} operations by @@ -81,11 +85,11 @@ public class RedisCache extends AbstractValueAdaptingCache { */ protected RedisCache(String name, RedisCacheWriter cacheWriter, RedisCacheConfiguration cacheConfiguration) { - super(cacheConfiguration.getAllowCacheNullValues()); + super(RedisAssertions.requireNonNull(cacheConfiguration, "CacheConfiguration must not be null") + .getAllowCacheNullValues()); Assert.notNull(name, "Name must not be null"); Assert.notNull(cacheWriter, "CacheWriter must not be null"); - Assert.notNull(cacheConfiguration, "CacheConfiguration must not be null"); this.name = name; this.cacheWriter = cacheWriter; @@ -116,7 +120,7 @@ protected RedisCacheWriter getCacheWriter() { * accessing entries in the cache. * * @return the configured {@link ConversionService} used to convert {@link Object cache keys} to a {@link String} when - * accessing entries in the cache. + * accessing entries in the cache. * @see RedisCacheConfiguration#getConversionService() * @see #getCacheConfiguration() */ @@ -163,7 +167,6 @@ private T getSynchronized(Object key, Callable valueLoader) { try { ValueWrapper result = get(key); - return result != null ? (T) result.get() : loadCacheValue(key, valueLoader); } finally { lock.unlock(); @@ -285,10 +288,19 @@ public void evict(Object key) { */ @Nullable protected Object preProcessCacheValue(@Nullable Object value) { - return value != null ? value : isAllowNullValues() ? NullValue.INSTANCE : null; } + @Override + public CompletableFuture retrieve(Object key) { + return super.retrieve(key); + } + + @Override + public CompletableFuture retrieve(Object key, Supplier> valueLoader) { + return super.retrieve(key, valueLoader); + } + /** * Serialize the given {@link String cache key}. * @@ -321,7 +333,7 @@ protected byte[] serializeCacheValue(Object value) { * * @param value array of bytes to deserialize; must not be {@literal null}. * @return an {@link Object} deserialized from the array of bytes using the configured value - * {@link RedisSerializationContext.SerializationPair}; can be {@literal null}. + * {@link RedisSerializationContext.SerializationPair}; can be {@literal null}. * @see RedisCacheConfiguration#getValueSerializationPair() */ @Nullable @@ -382,9 +394,8 @@ protected String convertKey(Object key) { return key.toString(); } - String message = String.format( - "Cannot convert cache key %s to String; Please register a suitable Converter" - + " via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'", + String message = String.format("Cannot convert cache key %s to String; Please register a suitable Converter" + + " via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'", source, key.getClass().getName()); throw new IllegalStateException(message); diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCacheManager.java b/src/main/java/org/springframework/data/redis/cache/RedisCacheManager.java index 9f0d42fa33..c64c84ac27 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCacheManager.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCacheManager.java @@ -32,12 +32,13 @@ import org.springframework.util.Assert; /** - * {@link CacheManager} backed by a {@link RedisCache}. + * {@link CacheManager} implementation for Redis backed by {@link RedisCache}. *

- * This {@link CacheManager} creates {@link Cache caches} by default upon first write. Empty {@link Cache caches} + * This {@link CacheManager} creates {@link Cache caches} on first write, by default. Empty {@link Cache caches} * are not visible in Redis due to how Redis represents empty data structures. *

- * {@link Cache Caches} requiring a different {@link RedisCacheConfiguration} than the default cache configuration + * {@link Cache Caches} requiring a different {@link RedisCacheConfiguration cache configuration} + * than the {@link RedisCacheConfiguration#defaultCacheConfig() default cache configuration} * can be specified via {@link RedisCacheManagerBuilder#withInitialCacheConfigurations(Map)} or individually * using {@link RedisCacheManagerBuilder#withCacheConfiguration(String, RedisCacheConfiguration)}. * @@ -45,7 +46,10 @@ * @author Mark Paluch * @author Yanming Zhou * @author John Blum + * @see org.springframework.cache.Cache + * @see org.springframework.cache.CacheManager * @see org.springframework.cache.transaction.AbstractTransactionSupportingCacheManager + * @see org.springframework.data.redis.connection.RedisConnectionFactory * @see org.springframework.data.redis.cache.RedisCacheConfiguration * @see org.springframework.data.redis.cache.RedisCacheWriter * @since 2.0 @@ -100,7 +104,8 @@ public static RedisCacheManagerBuilder builder(RedisConnectionFactory connection /** * Factory method used to construct a new {@link RedisCacheManager} initialized with - * the given {@link RedisConnectionFactory} and using the defaults for caching. + * the given {@link RedisConnectionFactory} and using {@link RedisCacheConfiguration#defaultCacheConfig() defaults} + * for caching. * *

*
locking
@@ -127,7 +132,7 @@ public static RedisCacheManager create(RedisConnectionFactory connectionFactory) Assert.notNull(connectionFactory, "ConnectionFactory must not be null"); - return new RedisCacheManager(RedisCacheWriter.nonLockingRedisCacheWriter(connectionFactory), + return new RedisCacheManager(org.springframework.data.redis.cache.RedisCacheWriter.nonLockingRedisCacheWriter(connectionFactory), RedisCacheConfiguration.defaultCacheConfig()); } @@ -359,6 +364,17 @@ protected Map getInitialCacheConfiguration() { return Collections.unmodifiableMap(this.initialCacheConfiguration); } + /** + * Returns a reference to the configured {@link RedisCacheWriter} used to perform {@link RedisCache} operations, + * such as reading from and writing to the cache. + * + * @return a reference to the configured {@link RedisCacheWriter}. + * @see org.springframework.data.redis.cache.RedisCacheWriter + */ + protected RedisCacheWriter getCacheWriter() { + return this.cacheWriter; + } + @Override protected RedisCache getMissingCache(String name) { return isAllowRuntimeCacheCreation() ? createRedisCache(name, getDefaultCacheConfiguration()) : null; @@ -373,7 +389,7 @@ protected RedisCache getMissingCache(String name) { * @return a new {@link RedisCache} instance; never {@literal null}. */ protected RedisCache createRedisCache(String name, @Nullable RedisCacheConfiguration cacheConfiguration) { - return new RedisCache(name, cacheWriter, resolveCacheConfiguration(cacheConfiguration)); + return new RedisCache(name, getCacheWriter(), resolveCacheConfiguration(cacheConfiguration)); } @Override @@ -630,19 +646,19 @@ public RedisCacheManager build() { Assert.state(cacheWriter != null, "CacheWriter must not be null;" + " You can provide one via 'RedisCacheManagerBuilder#cacheWriter(RedisCacheWriter)'"); - RedisCacheWriter resolvedCacheWriter = !CacheStatisticsCollector.none().equals(statisticsCollector) - ? cacheWriter.withStatisticsCollector(statisticsCollector) - : cacheWriter; + RedisCacheWriter resolvedCacheWriter = !CacheStatisticsCollector.none().equals(this.statisticsCollector) + ? this.cacheWriter.withStatisticsCollector(this.statisticsCollector) + : this.cacheWriter; RedisCacheManager cacheManager = newRedisCacheManager(resolvedCacheWriter); - cacheManager.setTransactionAware(enableTransactions); + cacheManager.setTransactionAware(this.enableTransactions); return cacheManager; } private RedisCacheManager newRedisCacheManager(RedisCacheWriter cacheWriter) { - return new RedisCacheManager(cacheWriter, cacheDefaults(), allowRuntimeCacheCreation, initialCaches); + return new RedisCacheManager(cacheWriter, cacheDefaults(), this.allowRuntimeCacheCreation, this.initialCaches); } } }