Review and polish for confiuring a cache lock TTL.

Closes #2300
Pull request: #2597.

Author: jxblum

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -31,29 +31,34 @@
 
 /**
  * {@link RedisCacheWriter} implementation capable of reading/writing binary data from/to Redis in {@literal standalone}
- * and {@literal cluster} environments. Works upon a given {@link RedisConnectionFactory} to obtain the actual
- * {@link RedisConnection}. <br />
+ * and {@literal cluster} environments, and uses a given {@link RedisConnectionFactory} to obtain the actual
+ * {@link RedisConnection}.
+ * <p>
  * {@link DefaultRedisCacheWriter} can be used in
- * {@link RedisCacheWriter#lockingRedisCacheWriter(RedisConnectionFactory) locking} or
- * {@link RedisCacheWriter#nonLockingRedisCacheWriter(RedisConnectionFactory) non-locking} mode. While
- * {@literal non-locking} aims for maximum performance it may result in overlapping, non atomic, command execution for
+ * {@link RedisCacheWriter#lockingRedisCacheWriter(RedisConnectionFactory) locking}
+ * or {@link RedisCacheWriter#nonLockingRedisCacheWriter(RedisConnectionFactory) non-locking} mode. While
+ * {@literal non-locking} aims for maximum performance it may result in overlapping, non-atomic, command execution for
  * operations spanning multiple Redis interactions like {@code putIfAbsent}. The {@literal locking} counterpart prevents
  * command overlap by setting an explicit lock key and checking against presence of this key which leads to additional
  * requests and potential command wait times.
  *
  * @author Christoph Strobl
  * @author Mark Paluch
  * @author André Prata
+ * @author John Blum
  * @since 2.0
  */
 class DefaultRedisCacheWriter implements RedisCacheWriter {
 
-	private final RedisConnectionFactory connectionFactory;
+	private final BatchStrategy batchStrategy;
+
+	private final CacheStatisticsCollector statistics;
+
 	private final Duration sleepTime;
 
+	private final RedisConnectionFactory connectionFactory;
+
 	private final TtlFunction lockTtl;
-	private final CacheStatisticsCollector statistics;
-	private final BatchStrategy batchStrategy;
 
 	/**
 	 * @param connectionFactory must not be {@literal null}.
@@ -166,8 +171,8 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Durat
 				}
 
 				return connection.get(key);
-			} finally {
 
+			} finally {
 				if (isLockingCacheWriter()) {
 					doUnlock(name, connection);
 				}
@@ -196,21 +201,21 @@ public void clean(String name, byte[] pattern) {
 			boolean wasLocked = false;
 
 			try {
-
 				if (isLockingCacheWriter()) {
 					doLock(name, name, pattern, connection);
 					wasLocked = true;
 				}
 
 				long deleteCount = batchStrategy.cleanCache(connection, name, pattern);
+
 				while (deleteCount > Integer.MAX_VALUE) {
 					statistics.incDeletesBy(name, Integer.MAX_VALUE);
 					deleteCount -= Integer.MAX_VALUE;
 				}
+
 				statistics.incDeletesBy(name, (int) deleteCount);
 
 			} finally {
-
 				if (wasLocked && isLockingCacheWriter()) {
 					doUnlock(name, connection);
 				}
@@ -280,8 +285,8 @@ private boolean isLockingCacheWriter() {
 	private <T> T execute(String name, Function<RedisConnection, T> callback) {
 
 		RedisConnection connection = connectionFactory.getConnection();
-		try {
 
+		try {
 			checkAndPotentiallyWaitUntilUnlocked(name, connection);
 			return callback.apply(connection);
 		} finally {
@@ -307,18 +312,19 @@ private void checkAndPotentiallyWaitUntilUnlocked(String name, RedisConnection c
 		}
 
 		long lockWaitTimeNs = System.nanoTime();
-		try {
 
+		try {
 			while (doCheckLock(name, connection)) {
 				Thread.sleep(sleepTime.toMillis());
 			}
-		} catch (InterruptedException ex) {
+		} catch (InterruptedException cause) {
 
 			// Re-interrupt current thread, to allow other participants to react.
 			Thread.currentThread().interrupt();
 
-			throw new PessimisticLockingFailureException(String.format("Interrupted while waiting to unlock cache %s", name),
-					ex);
+			String message = String.format("Interrupted while waiting to unlock cache %s", name);
+
+			throw new PessimisticLockingFailureException(message, cause);
 		} finally {
 			statistics.incLockTime(name, System.nanoTime() - lockWaitTimeNs);
 		}

src/main/java/org/springframework/data/redis/cache/SingletonTtlFunction.java renamed to src/main/java/org/springframework/data/redis/cache/FixedDurationTtlFunction.java

@@ -21,12 +21,16 @@
 import org.springframework.lang.Nullable;
 
 /**
- * Singleton implementation of {@link TtlFunction}.
+ * {@link TtlFunction} implementation returning the given, predetermined {@link Duration} used for per cache entry
+ * {@literal time-to-live (TTL) expiration}.
  *
  * @author Mark Paluch
+ * @author John Blum
+ * @see java.time.Duration
+ * @see org.springframework.data.redis.cache.RedisCacheWriter.TtlFunction
  * @since 3.2
  */
-public record SingletonTtlFunction(Duration duration) implements TtlFunction {
+public record FixedDurationTtlFunction(Duration duration) implements TtlFunction {
 
 	@Override
 	public Duration getTimeToLive(Object key, @Nullable Object value) {

src/main/java/org/springframework/data/redis/cache/RedisCache.java

@@ -67,10 +67,10 @@ public class RedisCache extends AbstractValueAdaptingCache {
 	 * Create a new {@link RedisCache}.
 	 *
 	 * @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 executing appropriate
-	 * Redis commands; must not be {@literal null}.
-	 * @param cacheConfiguration {@link RedisCacheConfiguration} applied to this {@link RedisCache on creation; must not
-	 * be {@literal null}.
+	 * @param cacheWriter {@link RedisCacheWriter} used to perform {@link RedisCache} operations by executing
+	 * the necessary Redis commands; must not be {@literal null}.
+	 * @param cacheConfiguration {@link RedisCacheConfiguration} applied to this {@link RedisCache} on creation;
+	 * must not be {@literal null}.
 	 * @throws IllegalArgumentException if either the given {@link RedisCacheWriter} or {@link RedisCacheConfiguration}
 	 * are {@literal null} or the given {@link String} name for this {@link RedisCache} is {@literal null}.
 	 */
@@ -88,18 +88,33 @@ protected RedisCache(String name, RedisCacheWriter cacheWriter, RedisCacheConfig
 	}
 
 	/**
-	 * Get {@link RedisCacheConfiguration} used.
+	 * Get the {@link RedisCacheConfiguration} used to configure this {@link RedisCache} on initialization.
 	 *
-	 * @return immutable {@link RedisCacheConfiguration}. Never {@literal null}.
+	 * @return an immutable {@link RedisCacheConfiguration} used to configure this {@link RedisCache} on initialization;
+	 * never {@literal null}.
 	 */
 	public RedisCacheConfiguration getCacheConfiguration() {
 		return this.cacheConfiguration;
 	}
 
+	/**
+	 * Gets the configured {@link RedisCacheWriter} used to modify Redis for cache operations.
+	 *
+	 * @return the configured {@link RedisCacheWriter} used to modify Redis for cache operations.
+	 */
 	protected RedisCacheWriter getCacheWriter() {
 		return this.cacheWriter;
 	}
 
+	/**
+	 * Gets the configured {@link ConversionService} used to convert {@link Object cache keys} to a {@link String}
+	 * when 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.
+	 * @see RedisCacheConfiguration#getConversionService()
+	 * @see #getCacheConfiguration()
+	 */
 	protected ConversionService getConversionService() {
 		return getCacheConfiguration().getConversionService();
 	}
@@ -111,12 +126,13 @@ public String getName() {
 
 	@Override
 	public RedisCacheWriter getNativeCache() {
-		return this.cacheWriter;
+		return getCacheWriter();
 	}
 
 	/**
-	 * Return the {@link CacheStatistics} snapshot for this cache instance. Statistics are accumulated per cache instance
-	 * and not from the backing Redis data store.
+	 * Return the {@link CacheStatistics} snapshot for this cache instance.
+	 * <p>
+	 * Statistics are accumulated per cache instance and not from the backing Redis data store.
 	 *
 	 * @return statistics object for this {@link RedisCache}.
 	 * @since 2.4
@@ -134,15 +150,25 @@ public <T> T get(Object key, Callable<T> valueLoader) {
 		return result != null ? (T) result.get() : getSynchronized(key, valueLoader);
 	}
 
-	@SuppressWarnings("unchecked")
 	@Nullable
+	@SuppressWarnings("unchecked")
 	private synchronized <T> T getSynchronized(Object key, Callable<T> valueLoader) {
 
 		ValueWrapper result = get(key);
 
 		return result != null ? (T) result.get() : loadCacheValue(key, valueLoader);
 	}
 
+	/**
+	 * Loads the {@link Object} using the given {@link Callable valueLoader} and {@link #put(Object, Object) puts}
+	 * the {@link Object loaded value} in the cache.
+	 *
+	 * @param <T> {@link Class type} of the loaded {@link Object cache value}.
+	 * @param key {@link Object key} mapped to the loaded {@link Object cache value}.
+	 * @param valueLoader {@link Callable} object used to load the {@link Object value}
+	 * for the given {@link Object key}.
+	 * @return the loaded {@link Object value}.
+	 */
 	protected <T> T loadCacheValue(Object key, Callable<T> valueLoader) {
 
 		T value;
@@ -172,7 +198,7 @@ public void put(Object key, @Nullable Object value) {
 
 		Object cacheValue = preProcessCacheValue(value);
 
-		if (!isAllowNullValues() && cacheValue == null) {
+		if (nullCacheValueIsNotAllowed(cacheValue)) {
 
 			String message = String.format("Cache '%s' does not allow 'null' values; Avoid storing null"
 				+ " via '@Cacheable(unless=\"#result == null\")' or configure RedisCache to allow 'null'"
@@ -182,20 +208,20 @@ public void put(Object key, @Nullable Object value) {
 		}
 
 		getCacheWriter().put(getName(), createAndConvertCacheKey(key), serializeCacheValue(cacheValue),
-			getCacheConfiguration().getTtl());
+			getCacheConfiguration().getTtlFunction().getTimeToLive(key, value));
 	}
 
 	@Override
 	public ValueWrapper putIfAbsent(Object key, @Nullable Object value) {
 
 		Object cacheValue = preProcessCacheValue(value);
 
-		if (!isAllowNullValues() && cacheValue == null) {
+		if (nullCacheValueIsNotAllowed(cacheValue)) {
 			return get(key);
 		}
 
 		byte[] result = getCacheWriter().putIfAbsent(getName(), createAndConvertCacheKey(key),
-			serializeCacheValue(cacheValue), getCacheConfiguration().getTtl());
+			serializeCacheValue(cacheValue), getCacheConfiguration().getTtlFunction().getTimeToLive(key, value));
 
 		return result != null ? new SimpleValueWrapper(fromStoreValue(deserializeCacheValue(result))) : null;
 	}
@@ -206,7 +232,7 @@ public void clear() {
 	}
 
 	/**
-	 * Clear keys that match the provided {@link String keyPattern}.
+	 * Clear keys that match the given {@link String keyPattern}.
 	 * <p>
 	 * Useful when cache keys are formatted in a style where Redis patterns can be used for matching these.
 	 *
@@ -303,7 +329,7 @@ protected String createCacheKey(Object key) {
 	}
 
 	/**
-	 * Convert {@code key} to a {@link String} representation used for cache key creation.
+	 * Convert {@code key} to a {@link String} used in cache key creation.
 	 *
 	 * @param key will never be {@literal null}.
 	 * @return never {@literal null}.
@@ -338,10 +364,9 @@ protected String convertKey(Object key) {
 			return key.toString();
 		}
 
-		String message = String.format(
-			"Cannot convert cache key %s to String; Please register a suitable Converter"
+		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());
+				source, key.getClass().getName());
 
 		throw new IllegalStateException(message);
 	}
@@ -403,4 +428,8 @@ private String prefixCacheKey(String key) {
 		// allow contextual cache names by computing the key prefix on every call.
 		return getCacheConfiguration().getKeyPrefixFor(getName()) + key;
 	}
+
+	private boolean nullCacheValueIsNotAllowed(@Nullable Object cacheValue) {
+		return cacheValue == null && !isAllowNullValues();
+	}
 }