DATAREDIS-1082 - Polishing.

Tweak documentation wording. Remove trailing whitespaces. Remove unused methods. Improve method names. Return zero stats when stats do not get collected.

Original Pull Request: #545

src/main/asciidoc/reference/redis.adoc

@@ -812,5 +812,6 @@ The following table lists the default settings for `RedisCacheConfiguration`:
 
 [NOTE]
 ====
-By default `RedisCache` does not expose statistics. Use `RedisCacheManagerBuilder.enableStatistics()` to collect local  _hits_ and _misses_ exposed via `RedisCache#getStatistics()`, that returns a snapshot of the collected data.
+By default `RedisCache`, statistics are disabled.
+Use `RedisCacheManagerBuilder.enableStatistics()` to collect local _hits_ and _misses_ through  `RedisCache#getStatistics()`, returning a snapshot of the collected data.
 ====

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

@@ -79,7 +79,7 @@ public interface CacheStatistics {
 	Instant getSince();
 
 	/**
-	 * @return instantaneous point in time of last statistics counter reset. Equals {@link #getSince()} if never resetted.
+	 * @return instantaneous point in time of last statistics counter reset. Equals {@link #getSince()} if never reset.
 	 */
 	Instant getLastReset();
 

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

@@ -26,35 +26,35 @@ public interface CacheStatisticsCollector extends CacheStatisticsProvider {
 
 	/**
 	 * Increase the counter for {@literal put operations} of the given cache.
-	 * 
+	 *
 	 * @param cacheName must not be {@literal null}.
 	 */
 	void incPuts(String cacheName);
 
 	/**
 	 * Increase the counter for {@literal get operations} of the given cache.
-	 * 
+	 *
 	 * @param cacheName must not be {@literal null}.
 	 */
 	void incGets(String cacheName);
 
 	/**
 	 * Increase the counter for {@literal get operations with result} of the given cache.
-	 * 
+	 *
 	 * @param cacheName must not be {@literal null}.
 	 */
 	void incHits(String cacheName);
 
 	/**
 	 * Increase the counter for {@literal get operations without result} of the given cache.
-	 * 
+	 *
 	 * @param cacheName must not be {@literal null}.
 	 */
 	void incMisses(String cacheName);
 
 	/**
 	 * Increase the counter for {@literal delete operations} of the given cache.
-	 * 
+	 *
 	 * @param cacheName must not be {@literal null}.
 	 */
 	default void incDeletes(String cacheName) {
@@ -63,21 +63,21 @@ public interface CacheStatisticsCollector extends CacheStatisticsProvider {
 
 	/**
 	 * Increase the counter for {@literal delete operations} of the given cache by the given value.
-	 * 
+	 *
 	 * @param cacheName must not be {@literal null}.
 	 */
 	void incDeletesBy(String cacheName, int value);
 
 	/**
 	 * Increase the gauge for {@literal sync lock duration} of the cache by the given nanoseconds.
-	 * 
+	 *
 	 * @param cacheName must not be {@literal null}.
 	 */
 	void incLockTime(String cacheName, long durationNS);
 
 	/**
 	 * Reset the all counters and gauges of for the given cache.
-	 * 
+	 *
 	 * @param cacheName must not be {@literal null}.
 	 */
 	void reset(String cacheName);
@@ -92,7 +92,7 @@ public interface CacheStatisticsCollector extends CacheStatisticsProvider {
 	/**
 	 * @return a default {@link CacheStatisticsCollector} implementation.
 	 */
-	static CacheStatisticsCollector instance/*clearlyNeedsBetterNaming*/() {
+	static CacheStatisticsCollector create() {
 		return new DefaultCacheStatisticsCollector();
 	}
 }

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

@@ -16,13 +16,18 @@
 package org.springframework.data.redis.cache;
 
 /**
+ * Interface to be implemented by objects that expose {@link CacheStatistics} identified by {@code cacheName}. Typically
+ * used by cache writers.
+ *
  * @author Christoph Strobl
+ * @author Mark Paluch
  * @since 2.4
  */
 public interface CacheStatisticsProvider {
 
 	/**
-	 * Obtain snapshot of the captured statistics.
+	 * Obtain snapshot of the captured statistics. May return a statistics object whose counters are zero if there are no
+	 * statistics for {@code cacheName}.
 	 *
 	 * @param cacheName must not be {@literal null}.
 	 * @return never {@literal null}.

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

@@ -21,7 +21,7 @@ import java.util.concurrent.ConcurrentHashMap;
 /**
  * Default {@link CacheStatisticsCollector} implementation holding synchronized per cache
  * {@link MutableCacheStatistics}.
- * 
+ *
  * @author Christoph Strobl
  * @since 2.4
  */

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

@@ -244,8 +244,12 @@ class DefaultRedisCacheWriter implements RedisCacheWriter {
 		statistics.reset(name);
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.redis.cache.RedisCacheWriter#with(CacheStatisticsCollector)
+	 */
 	@Override
-	public RedisCacheWriter with(CacheStatisticsCollector cacheStatisticsCollector) {
+	public RedisCacheWriter withStatisticsCollector(CacheStatisticsCollector cacheStatisticsCollector) {
 		return new DefaultRedisCacheWriter(connectionFactory, sleepTime, cacheStatisticsCollector);
 	}
 

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

@@ -115,10 +115,6 @@ class MutableCacheStatistics implements CacheStatistics {
 		return deletes.sum();
 	}
 
-	void incDeletes() {
-		incDeletes(1);
-	}
-
 	/**
 	 * @param x number of removals to add.
 	 */
@@ -138,11 +134,19 @@ class MutableCacheStatistics implements CacheStatistics {
 		return unit.convert(lockWaitTimeNs.sum(), TimeUnit.NANOSECONDS);
 	}
 
+	/* 
+	 * (non-Javadoc)
+	 * @see org.springframework.data.redis.cache.CacheStatistics#getSince()
+	 */
 	@Override
 	public Instant getSince() {
 		return this.aliveSince;
 	}
 
+	/* 
+	 * (non-Javadoc)
+	 * @see org.springframework.data.redis.cache.CacheStatistics#getLastReset()
+	 */
 	@Override
 	public Instant getLastReset() {
 		return lastReset;
@@ -271,6 +275,9 @@ class MutableCacheStatistics implements CacheStatistics {
 		 */
 		@Override
 		public long getLockWaitDuration(TimeUnit unit) {
+
+			Assert.notNull(unit, "TimeUnit must not be null");
+
 			return unit.convert(lockWaitTimeNS, TimeUnit.NANOSECONDS);
 		}
 

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

@@ -23,8 +23,9 @@ import org.springframework.util.ObjectUtils;
 /**
  * {@link CacheStatisticsCollector} implementation that does not capture anything but throws an
  * {@link IllegalStateException} when {@link #getCacheStatistics(String) obtaining} {@link CacheStatistics} for a cache.
- * 
+ *
  * @author Christoph Strobl
+ * @author Mark Paluch
  * @since 2.4
  */
 enum NoOpCacheStatisticsCollector implements CacheStatisticsCollector {
@@ -86,62 +87,102 @@ enum NoOpCacheStatisticsCollector implements CacheStatisticsCollector {
 	 */
 	@Override
 	public CacheStatistics getCacheStatistics(String cacheName) {
-		return new EmptyStats(cacheName);
+		return new EmptyStatistics(cacheName);
 	}
 
-	private static class EmptyStats implements CacheStatistics {
+	private static class EmptyStatistics implements CacheStatistics {
 
 		private final String cacheName;
 
-		EmptyStats(String cacheName) {
+		EmptyStatistics(String cacheName) {
 			this.cacheName = cacheName;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getCacheName()
+		 */
 		@Override
 		public String getCacheName() {
 			return cacheName;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getPuts()
+		 */
 		@Override
 		public long getPuts() {
-			return -1;
+			return 0;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getGets()
+		 */
 		@Override
 		public long getGets() {
-			return -1;
+			return 0;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getHits()
+		 */
 		@Override
 		public long getHits() {
-			return -1;
+			return 0;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getMisses()
+		 */
 		@Override
 		public long getMisses() {
-			return -1;
+			return 0;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getDeletes()
+		 */
 		@Override
 		public long getDeletes() {
-			return -1;
+			return 0;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getLockWaitDuration(java.util.concurrent.TimeUnit)
+		 */
 		@Override
 		public long getLockWaitDuration(TimeUnit unit) {
-			return -1;
+			return 0;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getSince()
+		 */
 		@Override
 		public Instant getSince() {
 			return Instant.EPOCH;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.redis.cache.CacheStatistics#getLastReset()
+		 */
 		@Override
 		public Instant getLastReset() {
 			return getSince();
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see java.lang.Object#equals(java.lang.Object)
+		 */
 		@Override
 		public boolean equals(Object o) {
 
@@ -152,10 +193,14 @@ enum NoOpCacheStatisticsCollector implements CacheStatisticsCollector {
 				return false;
 			}
 
-			EmptyStats that = (EmptyStats) o;
+			EmptyStatistics that = (EmptyStatistics) o;
 			return ObjectUtils.nullSafeEquals(cacheName, that.cacheName);
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see java.lang.Object#hashCode()
+		 */
 		@Override
 		public int hashCode() {
 			return ObjectUtils.nullSafeHashCode(cacheName);

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

@@ -194,12 +194,11 @@ public class RedisCache extends AbstractValueAdaptingCache {
 	}
 
 	/**
-	 * Return the {@link CacheStatistics} 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. Statistics are accumulated per cache instance
+	 * and not from the backing Redis data store.
 	 *
 	 * @return statistics object for this {@link RedisCache}.
 	 * @since 2.4
-	 * @throws IllegalStateException if the {@link RedisCacheWriter} does not support statistics.
 	 */
 	public CacheStatistics getStatistics() {
 		return cacheWriter.getCacheStatistics(getName());

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

@@ -462,17 +462,8 @@ public class RedisCacheManager extends AbstractTransactionSupportingCacheManager
 		 * @since 2.4
 		 */
 		public RedisCacheManagerBuilder enableStatistics() {
-			return statisticsCollector(CacheStatisticsCollector.instance());
-		}
 
-		/**
-		 * @return
-		 * @since 2.4
-		 */
-		private RedisCacheManagerBuilder statisticsCollector(CacheStatisticsCollector statisticsCollector) {
-
-			Assert.notNull(statisticsCollector, "StatisticsCollector must not be null!");
-			this.statisticsCollector = statisticsCollector;
+			this.statisticsCollector = CacheStatisticsCollector.create();
 			return this;
 		}
 
@@ -489,7 +480,7 @@ public class RedisCacheManager extends AbstractTransactionSupportingCacheManager
 			RedisCacheWriter theCacheWriter = cacheWriter;
 
 			if (!statisticsCollector.equals(CacheStatisticsCollector.none())) {
-				theCacheWriter = cacheWriter.with(statisticsCollector);
+				theCacheWriter = cacheWriter.withStatisticsCollector(statisticsCollector);
 			}
 
 			RedisCacheManager cm = new RedisCacheManager(theCacheWriter, defaultCacheConfiguration, initialCaches,

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

@@ -120,6 +120,6 @@ public interface RedisCacheWriter extends CacheStatisticsProvider {
 	 * @param cacheStatisticsCollector must not be {@literal null}.
 	 * @return new instance of {@link RedisCacheWriter}.
 	 */
-	RedisCacheWriter with(CacheStatisticsCollector cacheStatisticsCollector);
+	RedisCacheWriter withStatisticsCollector(CacheStatisticsCollector cacheStatisticsCollector);
 
 }

src/test/java/org/springframework/data/redis/cache/DefaultCacheStatisticsCollectorUnitTests.java

@@ -21,6 +21,8 @@ import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 /**
+ * Unit tests for {@link DefaultCacheStatisticsCollector}.
+ *
  * @author Christoph Strobl
  */
 class DefaultCacheStatisticsCollectorUnitTests {

src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java

@@ -90,7 +90,8 @@ public class DefaultRedisCacheWriterTests {
 	@Test // DATAREDIS-481, DATAREDIS-1082
 	public void putShouldAddEternalEntry() {
 
-		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory).with(CacheStatisticsCollector.instance());
+		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory)
+				.withStatisticsCollector(CacheStatisticsCollector.create());
 		writer.put(CACHE_NAME, binaryCacheKey, binaryCacheValue, Duration.ZERO);
 
 		doWithConnection(connection -> {
@@ -147,7 +148,8 @@ public class DefaultRedisCacheWriterTests {
 
 		doWithConnection(connection -> connection.set(binaryCacheKey, binaryCacheValue));
 
-		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory).with(CacheStatisticsCollector.instance());
+		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory)
+				.withStatisticsCollector(CacheStatisticsCollector.create());
 		assertThat(writer.get(CACHE_NAME, binaryCacheKey))
 				.isEqualTo(binaryCacheValue);
 
@@ -164,7 +166,8 @@ public class DefaultRedisCacheWriterTests {
 	@Test // DATAREDIS-481, DATAREDIS-1082
 	public void putIfAbsentShouldAddEternalEntryWhenKeyDoesNotExist() {
 
-		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory).with(CacheStatisticsCollector.instance());
+		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory)
+				.withStatisticsCollector(CacheStatisticsCollector.create());
 		assertThat(writer.putIfAbsent(CACHE_NAME, binaryCacheKey, binaryCacheValue,
 				Duration.ZERO)).isNull();
 
@@ -180,7 +183,8 @@ public class DefaultRedisCacheWriterTests {
 
 		doWithConnection(connection -> connection.set(binaryCacheKey, binaryCacheValue));
 
-		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory).with(CacheStatisticsCollector.instance());
+		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory)
+				.withStatisticsCollector(CacheStatisticsCollector.create());
 		assertThat(writer.putIfAbsent(CACHE_NAME, binaryCacheKey, "foo".getBytes(), Duration.ZERO))
 				.isEqualTo(binaryCacheValue);
 
@@ -194,7 +198,8 @@ public class DefaultRedisCacheWriterTests {
 	@Test // DATAREDIS-481, DATAREDIS-1082
 	public void putIfAbsentShouldAddExpiringEntryWhenKeyDoesNotExist() {
 
-		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory).with(CacheStatisticsCollector.instance());
+		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory)
+				.withStatisticsCollector(CacheStatisticsCollector.create());
 		assertThat(writer.putIfAbsent(CACHE_NAME, binaryCacheKey, binaryCacheValue, Duration.ofSeconds(5))).isNull();
 
 		doWithConnection(connection -> {
@@ -208,7 +213,8 @@ public class DefaultRedisCacheWriterTests {
 
 		doWithConnection(connection -> connection.set(binaryCacheKey, binaryCacheValue));
 
-		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory).with(CacheStatisticsCollector.instance());
+		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory)
+				.withStatisticsCollector(CacheStatisticsCollector.create());
 		writer.remove(CACHE_NAME, binaryCacheKey);
 
 		doWithConnection(connection -> assertThat(connection.exists(binaryCacheKey)).isFalse());
@@ -223,7 +229,8 @@ public class DefaultRedisCacheWriterTests {
 			connection.set("foo".getBytes(), "bar".getBytes());
 		});
 
-		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory).with(CacheStatisticsCollector.instance());
+		RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory)
+				.withStatisticsCollector(CacheStatisticsCollector.create());
 		writer.clean(CACHE_NAME, (CACHE_NAME + "::*").getBytes(Charset.forName("UTF-8")));
 
 		doWithConnection(connection -> {
@@ -261,7 +268,8 @@ public class DefaultRedisCacheWriterTests {
 	@Test // DATAREDIS-481, DATAREDIS-1082
 	public void lockingCacheWriterShouldWaitForLockRelease() throws InterruptedException {
 
-		DefaultRedisCacheWriter writer = (DefaultRedisCacheWriter) lockingRedisCacheWriter(connectionFactory).with(CacheStatisticsCollector.instance());
+		DefaultRedisCacheWriter writer = (DefaultRedisCacheWriter) lockingRedisCacheWriter(connectionFactory)
+				.withStatisticsCollector(CacheStatisticsCollector.create());
 		writer.lock(CACHE_NAME);
 
 		CountDownLatch beforeWrite = new CountDownLatch(1);
@@ -347,7 +355,7 @@ public class DefaultRedisCacheWriterTests {
 		cw.putIfAbsent(CACHE_NAME, binaryCacheKey, binaryCacheValue, Duration.ofSeconds(5));
 
 		assertThat(stats).isNotNull();
-		assertThat(stats.getPuts()).isNegative();
+		assertThat(stats.getPuts()).isZero();
 	}
 
 	private void doWithConnection(Consumer<RedisConnection> callback) {

src/test/java/org/springframework/data/redis/cache/MutableCacheStatisticsUnitTests.java

@@ -73,7 +73,7 @@ class MutableCacheStatisticsUnitTests {
 
 		assertThat(statistics.getDeletes()).isZero();
 
-		statistics.incDeletes();
+		statistics.incDeletes(1);
 
 		assertThat(statistics.getDeletes()).isOne();
 	}

src/test/java/org/springframework/data/redis/cache/RedisCacheManagerUnitTests.java

@@ -175,10 +175,10 @@ class RedisCacheManagerUnitTests {
 	@Test // DATAREDIS-1082
 	void builderSetsStatisticsCollectorWhenEnabled() {
 
-		when(cacheWriter.with(any())).thenReturn(cacheWriter);
+		when(cacheWriter.withStatisticsCollector(any())).thenReturn(cacheWriter);
 		RedisCacheManager.builder(cacheWriter).enableStatistics().build();
 
-		verify(cacheWriter).with(any(DefaultCacheStatisticsCollector.class));
+		verify(cacheWriter).withStatisticsCollector(any(DefaultCacheStatisticsCollector.class));
 	}
 
 	@Test // DATAREDIS-1082
@@ -186,6 +186,6 @@ class RedisCacheManagerUnitTests {
 
 		RedisCacheManager.builder(cacheWriter).build();
 
-		verify(cacheWriter, never()).with(any());
+		verify(cacheWriter, never()).withStatisticsCollector(any());
 	}
 }