DATAREDIS-481 - Optimize cache locking, update reference documentation, rename tests classes.

Author: christophstrobl

Diff for: src/main/asciidoc/reference/redis.adoc

@@ -465,30 +465,90 @@ As shown in the example above, the consuming code is decoupled from the actual s
 [[redis:support:cache-abstraction]]
 === Support for Spring Cache Abstraction
 
+NOTE: Changed in 2.0
+
 Spring Redis provides an implementation for Spring http://docs.spring.io/spring/docs/current/spring-framework-reference/html/cache.html[cache abstraction] through the `org.springframework.data.redis.cache` package. To use Redis as a backing implementation, simply add `RedisCacheManager` to your configuration:
 
-[source,xml]
+[source,java]
+----
+@Bean
+public RedisCacheManager cacheManager(RedisConnectionFactory connectionFactory) {
+	return RedisCacheManager.defaultCacheManager(connectionFactory);
+}
 ----
-<beans xmlns="http://www.springframework.org/schema/beans"
-  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-  xmlns:cache="http://www.springframework.org/schema/cache"
-  xmlns:c="http://www.springframework.org/schema/c"
-  xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd
-    http://www.springframework.org/schema/cache http://www.springframework.org/schema/cache/spring-cache.xsd">
 
-  <!-- turn on declarative caching -->
-  <cache:annotation-driven />
+`RedisCacheManager` behavior can be configured via `RedisCacheManagerConfigurator` allowing to set the default `RedisCacheConfiguration`, transaction behaviour and predefined caches.
 
-  <!-- declare Redis Cache Manager -->
-  <bean id="cacheManager" class="org.springframework.data.redis.cache.RedisCacheManager" c:template-ref="redisTemplate"/>
-</beans>
+[source,java]
+----
+RedisCacheManager cm = RedisCacheManager.usingRawConnectionFactory(connectionFactory)
+	.withCacheDefaults(defaultCacheConfig())
+	.withInitialCacheConfigurations(singletonMap("predefined", defaultCacheConfig().disableCachingNullValues()))
+	.transactionAware()
+	.createAndGet();
+----
+
+Behavior of `RedisCache` created via `RedisCacheManager` is defined via `RedisCacheConfiguration`. The configuration allows to set key expiration times, prefixes and ``RedisSerializer``s for converting to and from the binary storage format.
+As shown above `RedisCacheManager` allows definition of configurations on a per cache base.
+
+[source,java]
+----
+RedisCacheConfiguration config = RedisCacheConfiguration.defaultCacheConfig()
+    .entryTtl(Duration.ofSeconds(1))
+	.disableCachingNullValues();
+----
+
+Using default `RedisCacheManager` uses a non locking `RedisCacheWriter` for reading & writing bits.
+While this ensures a max of performance the lack of entry locking can lead to overlapping, non atomic commands, for `putIfAbsent` and `clean` as those methods combine a series of commands sent to Redis.
+The 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.
+
+It is possible to opt in to the locking behavior as follows:
+
+[source,java]
 ----
+RedisCacheManager cm = RedisCacheManager.usingCacheWriter(DefaultRedisCacheWriter.lockingRedisCacheWriter())
+	.withCacheDefaults(defaultCacheConfig())
+	...
+----
+
+.RedisCacheManager defaults
+[width="80%",cols="<1,<2",options="header"]
+|====
+|Setting
+|Value
+
+|Cache Writer
+|non locking
 
-NOTE: By default `RedisCacheManager` will lazily initialize `RedisCache` whenever a `Cache` is requested. This can be changed by predefining a `Set` of cache names.
+|Cache Configuration
+|`RedisCacheConfiguraiton#defaultConfiguration`
 
-NOTE: By default `RedisCacheManager` will not participate in any ongoing transaction. Use `setTransactionAware` to enable transaction support.
+|Initial Caches
+|none
 
-NOTE: By default `RedisCacheManager` does not prefix keys for cache regions, which can lead to an unexpected growth of a `ZSET` used to maintain known keys. It's highly recommended to enable the usage of prefixes in order to avoid this unexpected growth and potential key clashes using more than one cache region.
+|Trasaction Aware
+|no
+|====
+
+.RedisCacheConfiguration defaults
+[width="80%",cols="<1,<2",options="header"]
+|====
+|Key Expiration
+|none
+
+|Cache `null`
+|yes
 
-NOTE: By default `RedisCache` will not cache any `null` values as keys without a value get dropped by Redis itself. However you can explicitly enable `null` value caching via `RedisCacheManager` which will store `org.springframework.cache.support.NullValue` as a placeholder.
+|Prefix Keys
+|yes
+
+|Default Prefix
+|the actual cache name
+
+|Key Serializer
+|`StringRedisSerializer`
+
+|Value Serializer
+|`JdkSerializationRedisSerializer`
+|====
 

Diff for: src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -30,12 +30,18 @@
 /**
  * {@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}.
+ * {@link RedisConnection}. <br />
+ * {@link DefaultRedisCacheWriter} can be used in {@link #lockingRedisCacheWriter(RedisConnectionFactory) locking} or
+ * {@link #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
  * @since 2.0
  */
-class DefaultRedisCacheWriter implements RedisCacheWriter {
+public class DefaultRedisCacheWriter implements RedisCacheWriter {
 
 	private static final byte[] CLEAN_SCRIPT = "local keys = redis.call('KEYS', ARGV[1]); local keysCount = table.getn(keys); if(keysCount > 0) then for _, key in ipairs(keys) do redis.call('del', key); end; end; return keysCount;"
 			.getBytes(Charset.forName("UTF-8"));
@@ -64,12 +70,24 @@ class DefaultRedisCacheWriter implements RedisCacheWriter {
 		this.sleepTime = sleepTime;
 	}
 
+	/**
+	 * Create new {@link RedisCacheWriter} without locking behavior.
+	 *
+	 * @param connectionFactory must not be {@literal null}.
+	 * @return new instance of {@link DefaultRedisCacheWriter}.
+	 */
 	public static DefaultRedisCacheWriter nonLockingRedisCacheWriter(RedisConnectionFactory connectionFactory) {
 
 		Assert.notNull(connectionFactory, "ConnectionFactory must not be null!");
 		return new DefaultRedisCacheWriter(connectionFactory);
 	}
 
+	/**
+	 * Create new {@link RedisCacheWriter} with locking behavior.
+	 *
+	 * @param connectionFactory must not be {@literal null}.
+	 * @return new instance of {@link DefaultRedisCacheWriter}.
+	 */
 	public static DefaultRedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectionFactory) {
 
 		Assert.notNull(connectionFactory, "ConnectionFactory must not be null!");
@@ -115,15 +133,26 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, Duration ttl) {
 
 		return execute(name, connection -> {
 
-			if (connection.setNX(key, value)) {
+			if (isLockingCacheWriter()) {
+				doLock(name, connection);
+			}
 
-				if (shouldExpireWithin(ttl)) {
-					connection.pExpire(key, ttl.toMillis());
+			try {
+				if (connection.setNX(key, value)) {
+
+					if (shouldExpireWithin(ttl)) {
+						connection.pExpire(key, ttl.toMillis());
+					}
+					return null;
 				}
-				return null;
-			}
 
-			return connection.get(key);
+				return connection.get(key);
+			} finally {
+
+				if (isLockingCacheWriter()) {
+					doUnlock(name, connection);
+				}
+			}
 		});
 	}
 
@@ -184,21 +213,24 @@ public void clean(String name, byte[] pattern) {
 
 		execute(name, connection -> {
 
-			if (isLockingCacheWriter()) {
-				doLock(name, connection);
-			}
+			boolean wasLocked = false;
 
 			try {
 				if (connection instanceof RedisClusterConnection) {
 
+					if (isLockingCacheWriter()) {
+						doLock(name, connection);
+						wasLocked = true;
+					}
+
 					byte[][] keys = connection.keys(pattern).stream().toArray(size -> new byte[size][]);
 					connection.del(keys);
 				} else {
 					connection.eval(CLEAN_SCRIPT, ReturnType.INTEGER, 0, pattern);
 				}
 			} finally {
 
-				if (isLockingCacheWriter()) {
+				if (wasLocked && isLockingCacheWriter()) {
 					doUnlock(name, connection);
 				}
 			}
@@ -262,11 +294,12 @@ byte[] createCacheLockKey(String name) {
 	}
 
 	/**
-	 * @author Christoph Strobl
 	 * @param <T>
+	 * @author Christoph Strobl
 	 * @since 2.0
 	 */
 	interface ConnectionCallback<T> {
+
 		T doWithConnection(RedisConnection connection);
 	}
 }

Diff for: src/main/java/org/springframework/data/redis/cache/RedisCache.java

@@ -63,7 +63,7 @@ public class RedisCache extends AbstractValueAdaptingCache {
 	 * @param cacheWriter must not be {@literal null}.
 	 * @param cacheConfig must not be {@literal null}.
 	 */
-	public RedisCache(String name, RedisCacheWriter cacheWriter, RedisCacheConfiguration cacheConfig) {
+	RedisCache(String name, RedisCacheWriter cacheWriter, RedisCacheConfiguration cacheConfig) {
 
 		super(cacheConfig.getAllowCacheNullValues());
 
@@ -227,11 +227,13 @@ protected Object deserializeCacheValue(byte[] value) {
 		return cacheConfig.getValueSerializationPair().read(ByteBuffer.wrap(value));
 	}
 
-	private byte[] createAndConvertCacheKey(Object key) {
-		return serializeCacheKey(createCacheKey(key));
-	}
-
-	private String createCacheKey(Object key) {
+	/**
+	 * Customization hook for creating cache key before it gets serialized.
+	 *
+	 * @param key will never be {@literal null}.
+	 * @return never {@literal null}.
+	 */
+	protected String createCacheKey(Object key) {
 
 		String convertedKey = conversionService.convert(key, String.class);
 		if (!cacheConfig.usePrefix()) {
@@ -241,6 +243,10 @@ private String createCacheKey(Object key) {
 		return prefixCacheKey(convertedKey);
 	}
 
+	private byte[] createAndConvertCacheKey(Object key) {
+		return serializeCacheKey(createCacheKey(key));
+	}
+
 	private String prefixCacheKey(String key) {
 		return cacheConfig.getKeyPrefix().orElseGet(() -> name + "::") + key;
 	}

Diff for: src/main/java/org/springframework/data/redis/cache/RedisCacheManager.java

@@ -294,7 +294,7 @@ public RedisCacheManagerConfigurator withInitialCacheConfigurations(
 		 *
 		 * @return new instance of {@link RedisCacheManager}.
 		 */
-		public RedisCacheManager build() {
+		public RedisCacheManager createAndGet() {
 
 			RedisCacheManager cm = new RedisCacheManager(cacheWriter, defaultCacheConfiguration, intialCaches) {
 

Diff for: src/test/java/org/springframework/data/redis/cache/NRedisCacheManagerUnitTests.java renamed to src/test/java/org/springframework/data/redis/cache/RedisCacheManagerUnitTests.java

@@ -25,13 +25,15 @@
 import org.mockito.junit.MockitoJUnitRunner;
 import org.springframework.cache.Cache;
 import org.springframework.cache.transaction.TransactionAwareCacheDecorator;
+import org.springframework.context.annotation.Bean;
+import org.springframework.data.redis.connection.RedisConnectionFactory;
 import org.springframework.test.util.ReflectionTestUtils;
 
 /**
  * @author Christoph Strobl
  */
 @RunWith(MockitoJUnitRunner.class)
-public class NRedisCacheManagerUnitTests {
+public class RedisCacheManagerUnitTests {
 
 	@Mock RedisCacheWriter cacheWriter;
 
@@ -40,7 +42,7 @@ public void missingCacheShouldBeCreatedWithDefaultConfiguration() {
 
 		RedisCacheConfiguration configuration = RedisCacheConfiguration.defaultCacheConfig().disableKeyPrefix();
 
-		RedisCacheManager cm = RedisCacheManager.usingCacheWriter(cacheWriter).withCacheDefaults(configuration).build();
+		RedisCacheManager cm = RedisCacheManager.usingCacheWriter(cacheWriter).withCacheDefaults(configuration).createAndGet();
 		assertThat(cm.getMissingCache("new-cache").getCacheConfiguration()).isEqualTo(configuration);
 	}
 
@@ -50,7 +52,7 @@ public void predefinedCacheShouldBeCreatedWithSpecificConfig() {
 		RedisCacheConfiguration configuration = RedisCacheConfiguration.defaultCacheConfig().disableKeyPrefix();
 
 		RedisCacheManager cm = RedisCacheManager.usingCacheWriter(cacheWriter)
-				.withInitialCacheConfigurations(Collections.singletonMap("predefined-cache", configuration)).build();
+				.withInitialCacheConfigurations(Collections.singletonMap("predefined-cache", configuration)).createAndGet();
 
 		assertThat(((RedisCache) cm.getCache("predefined-cache")).getCacheConfiguration()).isEqualTo(configuration);
 		assertThat(cm.getMissingCache("new-cache").getCacheConfiguration()).isNotEqualTo(configuration);
@@ -59,11 +61,20 @@ public void predefinedCacheShouldBeCreatedWithSpecificConfig() {
 	@Test // DATAREDIS-481
 	public void transactionAwareCacheManagerShouldDecoracteCache() {
 
-		Cache cache = RedisCacheManager.usingCacheWriter(cacheWriter).transactionAware().build()
+		Cache cache = RedisCacheManager.usingCacheWriter(cacheWriter).transactionAware().createAndGet()
 				.getCache("decoracted-cache");
 
 		assertThat(cache).isInstanceOfAny(TransactionAwareCacheDecorator.class);
 		assertThat(ReflectionTestUtils.getField(cache, "targetCache")).isInstanceOf(RedisCache.class);
 	}
 
+	@Bean
+	public RedisCacheManager cacheManager(RedisConnectionFactory connectionFactory) {
+		return RedisCacheManager.usingRawConnectionFactory(connectionFactory)
+				.withCacheDefaults(RedisCacheConfiguration.defaultCacheConfig())
+				.withInitialCacheConfigurations(Collections.singletonMap("predefined", RedisCacheConfiguration.defaultCacheConfig().disableCachingNullValues()))
+				.transactionAware()
+				.createAndGet();
+	}
+
 }

Diff for: src/test/java/org/springframework/data/redis/cache/NRedisCacheTests.java renamed to src/test/java/org/springframework/data/redis/cache/RedisCacheTests.java

@@ -50,7 +50,7 @@
  * @author Christoph Strobl
  */
 @RunWith(Parameterized.class)
-public class NRedisCacheTests {
+public class RedisCacheTests {
 
 	String key = "key-1";
 	String cacheKey = "cache::" + key;
@@ -66,7 +66,7 @@ public class NRedisCacheTests {
 	RedisSerializer serializer;
 	RedisCache cache;
 
-	public NRedisCacheTests(RedisConnectionFactory connectionFactory, RedisSerializer serializer) {
+	public RedisCacheTests(RedisConnectionFactory connectionFactory, RedisSerializer serializer) {
 
 		this.connectionFactory = connectionFactory;
 		this.serializer = serializer;