diff --git a/pom.xml b/pom.xml index 09bebeb5bd..2974a12126 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-redis - 3.4.0-SNAPSHOT + 3.4.0-GH-2986-SNAPSHOT Spring Data Redis Spring Data module for Redis diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java index ace9d4acb6..625c085729 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java @@ -805,13 +805,11 @@ public void returnResourceForSpecificNode(RedisClusterNode node, Object client) */ public static class JedisClusterTopologyProvider implements ClusterTopologyProvider { - private long time = 0; + private final JedisCluster cluster; private final long cacheTimeMs; - private @Nullable ClusterTopology cached; - - private final JedisCluster cluster; + private volatile @Nullable JedisClusterTopology cached; /** * Create new {@link JedisClusterTopologyProvider}. Uses a default cache timeout of 100 milliseconds. @@ -842,12 +840,12 @@ public JedisClusterTopologyProvider(JedisCluster cluster, Duration cacheTimeout) @Override public ClusterTopology getTopology() { - if (cached != null && shouldUseCachedValue()) { - return cached; + JedisClusterTopology topology = cached; + if (shouldUseCachedValue(topology)) { + return topology; } Map errors = new LinkedHashMap<>(); - List> list = new ArrayList<>(cluster.getClusterNodes().entrySet()); Collections.shuffle(list); @@ -856,13 +854,10 @@ public ClusterTopology getTopology() { try (Connection connection = entry.getValue().getResource()) { - time = System.currentTimeMillis(); Set nodes = Converters.toSetOfRedisClusterNodes(new Jedis(connection).clusterNodes()); - - cached = new ClusterTopology(nodes); - - return cached; + topology = cached = new JedisClusterTopology(nodes, System.currentTimeMillis()); + return topology; } catch (Exception ex) { errors.put(entry.getKey(), ex); @@ -887,9 +882,38 @@ public ClusterTopology getTopology() { * topology. * @see #JedisClusterTopologyProvider(JedisCluster, Duration) * @since 2.2 + * @deprecated since 3.3.4, use {@link #shouldUseCachedValue(JedisClusterTopology)} instead. */ + @Deprecated(since = "3.3.4") protected boolean shouldUseCachedValue() { - return time + cacheTimeMs > System.currentTimeMillis(); + return false; + } + + /** + * Returns whether {@link #getTopology()} should return the cached {@link JedisClusterTopology}. Uses a time-based + * caching. + * + * @return {@literal true} to use the cached {@link ClusterTopology}; {@literal false} to fetch a new cluster + * topology. + * @see #JedisClusterTopologyProvider(JedisCluster, Duration) + * @since 3.3.4 + */ + protected boolean shouldUseCachedValue(@Nullable JedisClusterTopology topology) { + return topology != null && topology.getTime() + cacheTimeMs > System.currentTimeMillis(); + } + } + + protected static class JedisClusterTopology extends ClusterTopology { + + private final long time; + + public JedisClusterTopology(Set nodes, long time) { + super(nodes); + this.time = time; + } + + public long getTime() { + return time; } } diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionTests.java index 0ebc8ea86d..c56332f5f5 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionTests.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionTests.java @@ -43,6 +43,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.extension.ExtendWith; + import org.springframework.dao.DataAccessException; import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.domain.Range.Bound; @@ -53,6 +54,7 @@ import org.springframework.data.redis.connection.BitFieldSubCommands; import org.springframework.data.redis.connection.ClusterConnectionTests; import org.springframework.data.redis.connection.ClusterSlotHashUtil; +import org.springframework.data.redis.connection.ClusterTopology; import org.springframework.data.redis.connection.DataType; import org.springframework.data.redis.connection.DefaultSortParameters; import org.springframework.data.redis.connection.Limit; @@ -75,6 +77,7 @@ import org.springframework.data.redis.test.condition.EnabledOnRedisClusterAvailable; import org.springframework.data.redis.test.extension.JedisExtension; import org.springframework.data.redis.test.util.HexStringUtils; +import org.springframework.test.util.ReflectionTestUtils; /** * @author Christoph Strobl @@ -2950,4 +2953,20 @@ void lPosNonExisting() { assertThat(result).isEmpty(); } + + @Test // GH-2986 + void shouldUseCachedTopology() { + + JedisClusterConnection.JedisClusterTopologyProvider provider = (JedisClusterConnection.JedisClusterTopologyProvider) clusterConnection + .getTopologyProvider(); + ReflectionTestUtils.setField(provider, "cached", null); + + ClusterTopology topology = provider.getTopology(); + assertThat(topology).isInstanceOf(JedisClusterConnection.JedisClusterTopology.class); + + assertThat(provider.shouldUseCachedValue(null)).isFalse(); + assertThat(provider.shouldUseCachedValue(new JedisClusterConnection.JedisClusterTopology(Set.of(), 0))).isFalse(); + assertThat(provider.shouldUseCachedValue( + new JedisClusterConnection.JedisClusterTopology(Set.of(), System.currentTimeMillis() + 100))).isTrue(); + } }