Improve redis repository implementations

Storing values in zset makes them less prone to races.

Fixes gh-929

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMetricRepository.java

@@ -28,8 +28,6 @@ import org.springframework.boot.actuate.metrics.writer.Delta;
 import org.springframework.data.redis.connection.RedisConnectionFactory;
 import org.springframework.data.redis.core.BoundZSetOperations;
 import org.springframework.data.redis.core.RedisOperations;
-import org.springframework.data.redis.core.RedisTemplate;
-import org.springframework.data.redis.core.ValueOperations;
 import org.springframework.util.Assert;
 
 /**
@@ -51,14 +49,9 @@ public class RedisMetricRepository implements MetricRepository {
 
 	private final RedisOperations<String, String> redisOperations;
 
-	private final ValueOperations<String, Long> longOperations;
-
 	public RedisMetricRepository(RedisConnectionFactory redisConnectionFactory) {
 		Assert.notNull(redisConnectionFactory, "RedisConnectionFactory must not be null");
 		this.redisOperations = RedisUtils.stringTemplate(redisConnectionFactory);
-		RedisTemplate<String, Long> longRedisTemplate = RedisUtils.createRedisTemplate(
-				redisConnectionFactory, Long.class);
-		this.longOperations = longRedisTemplate.opsForValue();
 		this.zSetOperations = this.redisOperations.boundZSetOps(this.key);
 	}
 
@@ -89,7 +82,7 @@ public class RedisMetricRepository implements MetricRepository {
 	public Metric<?> findOne(String metricName) {
 		String redisKey = keyFor(metricName);
 		String raw = this.redisOperations.opsForValue().get(redisKey);
-		return deserialize(redisKey, raw);
+		return deserialize(redisKey, raw, this.zSetOperations.score(redisKey));
 	}
 
 	@Override
@@ -102,7 +95,8 @@ public class RedisMetricRepository implements MetricRepository {
 		List<Metric<?>> result = new ArrayList<Metric<?>>(keys.size());
 		List<String> values = this.redisOperations.opsForValue().multiGet(keys);
 		for (String v : values) {
-			Metric<?> value = deserialize(keysIt.next(), v);
+			String key = keysIt.next();
+			Metric<?> value = deserialize(key, v, this.zSetOperations.score(key));
 			if (value != null) {
 				result.add(value);
 			}
@@ -121,15 +115,19 @@ public class RedisMetricRepository implements MetricRepository {
 		String name = delta.getName();
 		String key = keyFor(name);
 		trackMembership(key);
-		this.longOperations.increment(key, delta.getValue().longValue());
+		double value = this.zSetOperations.incrementScore(key, delta.getValue()
+				.doubleValue());
+		String raw = serialize(new Metric<Double>(name, value, delta.getTimestamp()));
+		this.redisOperations.opsForValue().set(key, raw);
 	}
 
 	@Override
 	public void set(Metric<?> value) {
-		String raw = serialize(value);
 		String name = value.getName();
 		String key = keyFor(name);
 		trackMembership(key);
+		this.zSetOperations.add(key, value.getValue().doubleValue());
+		String raw = serialize(value);
 		this.redisOperations.opsForValue().set(key, raw);
 	}
 
@@ -141,18 +139,16 @@ public class RedisMetricRepository implements MetricRepository {
 		}
 	}
 
-	private Metric<?> deserialize(String redisKey, String v) {
+	private Metric<?> deserialize(String redisKey, String v, Double value) {
 		if (redisKey == null || v == null || !redisKey.startsWith(this.prefix)) {
 			return null;
 		}
-		String[] vals = v.split("@");
-		Double value = Double.valueOf(vals[0]);
-		Date timestamp = vals.length > 1 ? new Date(Long.valueOf(vals[1])) : new Date();
+		Date timestamp = new Date(Long.valueOf(v));
 		return new Metric<Double>(nameFor(redisKey), value, timestamp);
 	}
 
 	private String serialize(Metric<?> entity) {
-		return String.valueOf(entity.getValue()) + "@" + entity.getTimestamp().getTime();
+		return String.valueOf(entity.getTimestamp().getTime());
 	}
 
 	private String keyFor(String name) {
@@ -164,7 +160,7 @@ public class RedisMetricRepository implements MetricRepository {
 	}
 
 	private void trackMembership(String redisKey) {
-		this.zSetOperations.add(redisKey, 0.0D);
+		this.zSetOperations.incrementScore(redisKey, 0.0D);
 	}
 
 }

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepository.java

@@ -33,9 +33,9 @@ import org.springframework.util.Assert;
 
 /**
  * {@link MultiMetricRepository} implementation backed by a redis store. Metric values are
- * stored as regular values against a key composed of the group name prefixed with a
- * constant prefix (default "spring.groups."). The group names are stored as a zset under
- * "keys." + <code>[prefix]</code>.
+ * stored as zset values and the timestamps as regular values, both against a key composed
+ * of the group name prefixed with a constant prefix (default "spring.groups."). The group
+ * names are stored as a zset under "keys." + <code>[prefix]</code>.
  * 
  * @author Dave Syer
  */
@@ -167,7 +167,7 @@ public class RedisMultiMetricRepository implements MultiMetricRepository {
 	}
 
 	private void trackMembership(String redisKey) {
-		this.zSetOperations.add(redisKey, 0.0D);
+		this.zSetOperations.incrementScore(redisKey, 0.0D);
 	}
 
 }

spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMetricRepositoryTests.java

@@ -70,6 +70,15 @@ public class RedisMetricRepositoryTests {
 		assertEquals(3, this.repository.findOne("foo").getValue().longValue());
 	}
 
+	@Test
+	public void setIncrementAndGet() {
+		this.repository.set(new Metric<Number>("foo", 12.3));
+		this.repository.increment(new Delta<Long>("foo", 3L));
+		Metric<?> metric = this.repository.findOne("foo");
+		assertEquals("foo", metric.getName());
+		assertEquals(15.3, metric.getValue().doubleValue(), 0.01);
+	}
+
 	@Test
 	public void findAll() {
 		this.repository.increment(new Delta<Long>("foo", 3L));

spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepositoryTests.java

@@ -81,6 +81,16 @@ public class RedisMultiMetricRepositoryTests {
 
 	@Test
 	public void setAndGet() {
+		this.repository.set("foo",
+				Arrays.<Metric<?>> asList(new Metric<Number>("foo.bar", 12.3)));
+		this.repository.set("foo",
+				Arrays.<Metric<?>> asList(new Metric<Number>("foo.bar", 15.3)));
+		assertEquals(15.3, Iterables.collection(this.repository.findAll("foo"))
+				.iterator().next().getValue());
+	}
+
+	@Test
+	public void setAndGetMultiple() {
 		this.repository.set("foo", Arrays.<Metric<?>> asList(new Metric<Number>(
 				"foo.val", 12.3), new Metric<Number>("foo.bar", 11.3)));
 		assertEquals(2, Iterables.collection(this.repository.findAll("foo")).size());