From b12d7c705cc8ba9012fb46a61cde50fbe5c812a9 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Thu, 22 May 2014 12:39:04 +0100 Subject: [PATCH] Improve redis repository implementations Storing values in zset makes them less prone to races. Fixes gh-929 --- .../redis/RedisMetricRepository.java | 30 ++++++++----------- .../redis/RedisMultiMetricRepository.java | 8 ++--- .../redis/RedisMetricRepositoryTests.java | 9 ++++++ .../RedisMultiMetricRepositoryTests.java | 10 +++++++ 4 files changed, 36 insertions(+), 21 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMetricRepository.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMetricRepository.java index fb5ca2c0c3f..61453a6c3e6 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMetricRepository.java +++ b/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 redisOperations; - private final ValueOperations longOperations; - public RedisMetricRepository(RedisConnectionFactory redisConnectionFactory) { Assert.notNull(redisConnectionFactory, "RedisConnectionFactory must not be null"); this.redisOperations = RedisUtils.stringTemplate(redisConnectionFactory); - RedisTemplate 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> result = new ArrayList>(keys.size()); List 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(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(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); } } diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepository.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepository.java index f4b5ffb93d3..cf13aadef13 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepository.java +++ b/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." + [prefix]. + * 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." + [prefix]. * * @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); } } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMetricRepositoryTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMetricRepositoryTests.java index d3ac2e25f0c..1c7048960a7 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMetricRepositoryTests.java +++ b/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("foo", 12.3)); + this.repository.increment(new Delta("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("foo", 3L)); diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepositoryTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepositoryTests.java index 51d53946924..dd218b203e2 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepositoryTests.java +++ b/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.> asList(new Metric("foo.bar", 12.3))); + this.repository.set("foo", + Arrays.> asList(new Metric("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.> asList(new Metric( "foo.val", 12.3), new Metric("foo.bar", 11.3))); assertEquals(2, Iterables.collection(this.repository.findAll("foo")).size());