Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit f528585

Browse files
mp911dechristophstrobl
authored andcommitted
Use value object for topology caching.
We now use a value object for caching the topology to avoid races in updating the cache timestamp. Also, we set the cache timestamp after obtaining the topology to avoid that I/O latency expires the topology cache. Closes: #2986 Original Pull Request: #2989 
1 parent f9dd9bc commit f528585

File tree

2 files changed

+56
-13
lines changed

2 files changed

+56
-13
lines changed

‎src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java‎

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -805,13 +805,11 @@ public void returnResourceForSpecificNode(RedisClusterNode node, Object client)
805805
*/
806806
public static class JedisClusterTopologyProvider implements ClusterTopologyProvider {
807807

808-
private longtime = 0;
808+
private finalJedisClustercluster;
809809

810810
private final long cacheTimeMs;
811811

812-
private @Nullable ClusterTopology cached;
813-
814-
private final JedisCluster cluster;
812+
private volatile @Nullable JedisClusterTopology cached;
815813

816814
/**
817815
* Create new {@link JedisClusterTopologyProvider}. Uses a default cache timeout of 100 milliseconds.
@@ -842,12 +840,12 @@ public JedisClusterTopologyProvider(JedisCluster cluster, Duration cacheTimeout)
842840
@Override
843841
public ClusterTopology getTopology() {
844842

845-
if (cached != null && shouldUseCachedValue()) {
846-
return cached;
843+
JedisClusterTopology topology = cached;
844+
if (shouldUseCachedValue(topology)) {
845+
return topology;
847846
}
848847

849848
Map<String, Exception> errors = new LinkedHashMap<>();
850-
851849
List<Entry<String, ConnectionPool>> list = new ArrayList<>(cluster.getClusterNodes().entrySet());
852850

853851
Collections.shuffle(list);
@@ -856,13 +854,10 @@ public ClusterTopology getTopology() {
856854

857855
try (Connection connection = entry.getValue().getResource()) {
858856

859-
time = System.currentTimeMillis();
860857

861858
Set<RedisClusterNode> nodes = Converters.toSetOfRedisClusterNodes(new Jedis(connection).clusterNodes());
862-
863-
cached = new ClusterTopology(nodes);
864-
865-
return cached;
859+
topology = cached = new JedisClusterTopology(nodes, System.currentTimeMillis());
860+
return topology;
866861

867862
} catch (Exception ex) {
868863
errors.put(entry.getKey(), ex);
@@ -887,9 +882,38 @@ public ClusterTopology getTopology() {
887882
* topology.
888883
* @see #JedisClusterTopologyProvider(JedisCluster, Duration)
889884
* @since 2.2
885+
* @deprecated since 3.3.4, use {@link #shouldUseCachedValue(JedisClusterTopology)} instead.
890886
*/
887+
@Deprecated(since = "3.3.4")
891888
protected boolean shouldUseCachedValue() {
892-
return time + cacheTimeMs > System.currentTimeMillis();
889+
return false;
890+
}
891+
892+
/**
893+
* Returns whether {@link #getTopology()} should return the cached {@link JedisClusterTopology}. Uses a time-based
894+
* caching.
895+
*
896+
* @return {@literal true} to use the cached {@link ClusterTopology}; {@literal false} to fetch a new cluster
897+
* topology.
898+
* @see #JedisClusterTopologyProvider(JedisCluster, Duration)
899+
* @since 3.3.4
900+
*/
901+
protected boolean shouldUseCachedValue(@Nullable JedisClusterTopology topology) {
902+
return topology != null && topology.getTime() + cacheTimeMs > System.currentTimeMillis();
903+
}
904+
}
905+
906+
protected static class JedisClusterTopology extends ClusterTopology {
907+
908+
private final long time;
909+
910+
public JedisClusterTopology(Set<RedisClusterNode> nodes, long time) {
911+
super(nodes);
912+
this.time = time;
913+
}
914+
915+
public long getTime() {
916+
return time;
893917
}
894918
}
895919

‎src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionTests.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.junit.jupiter.api.Test;
4444
import org.junit.jupiter.api.TestInstance;
4545
import org.junit.jupiter.api.extension.ExtendWith;
46+
4647
import org.springframework.dao.DataAccessException;
4748
import org.springframework.dao.InvalidDataAccessApiUsageException;
4849
import org.springframework.data.domain.Range.Bound;
@@ -53,6 +54,7 @@
5354
import org.springframework.data.redis.connection.BitFieldSubCommands;
5455
import org.springframework.data.redis.connection.ClusterConnectionTests;
5556
import org.springframework.data.redis.connection.ClusterSlotHashUtil;
57+
import org.springframework.data.redis.connection.ClusterTopology;
5658
import org.springframework.data.redis.connection.DataType;
5759
import org.springframework.data.redis.connection.DefaultSortParameters;
5860
import org.springframework.data.redis.connection.Limit;
@@ -75,6 +77,7 @@
7577
import org.springframework.data.redis.test.condition.EnabledOnRedisClusterAvailable;
7678
import org.springframework.data.redis.test.extension.JedisExtension;
7779
import org.springframework.data.redis.test.util.HexStringUtils;
80+
import org.springframework.test.util.ReflectionTestUtils;
7881

7982
/**
8083
* @author Christoph Strobl
@@ -2950,4 +2953,20 @@ void lPosNonExisting() {
29502953

29512954
assertThat(result).isEmpty();
29522955
}
2956+
2957+
@Test // GH-2986
2958+
void shouldUseCachedTopology() {
2959+
2960+
JedisClusterConnection.JedisClusterTopologyProvider provider = (JedisClusterConnection.JedisClusterTopologyProvider) clusterConnection
2961+
.getTopologyProvider();
2962+
ReflectionTestUtils.setField(provider, "cached", null);
2963+
2964+
ClusterTopology topology = provider.getTopology();
2965+
assertThat(topology).isInstanceOf(JedisClusterConnection.JedisClusterTopology.class);
2966+
2967+
assertThat(provider.shouldUseCachedValue(null)).isFalse();
2968+
assertThat(provider.shouldUseCachedValue(new JedisClusterConnection.JedisClusterTopology(Set.of(), 0))).isFalse();
2969+
assertThat(provider.shouldUseCachedValue(
2970+
new JedisClusterConnection.JedisClusterTopology(Set.of(), System.currentTimeMillis() + 100))).isTrue();
2971+
}
29532972
}

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /