Use JedisCluster.psetex() in JedisClusterStringCommands.pSetEx() instead own dispatch via Jedis [DATAREDIS-588] #1163
Description
Amarjit Thiyam opened DATAREDIS-588 and commented
This is as per the discussion we had in jedis & then turns out to be spring-data-redis issue.
Please refer: redis/jedis#1456
We have 3 masters with 1 slave per master in a redis cluster where we use single key SET & GET operations only on our cluster.
The code - https://github.com/spring-projects/spring-data-redis/blob/master/src/main/java/org/springframework/data/redis/core/DefaultValueOperations.java#L192 looks for TimeUnit to be non MILLISECONDS & invokes a PsetX as we use MILLISECONDS as time unit with a cache ttl while set operation.
if (!TimeUnit.MILLISECONDS.equals(unit) || !failsafeInvokePsetEx(connection))
The JedisClusterConnection does getTopology()
topologyProvider.getTopology().getKeyServingMasterNode(key))
And I believe due to above, with 12+ clients (app servers) connected, we see 90+ connected clients for cluster/ping opertions. And in high load scenarios, we see issues and also cache SET is delayed due to a hop to refresh topology before actual SET operation.
Improvements/Issues:
-
While jedis does shuffle of nodes, why Spring redis for jedis does simply look for the first host:port for any cluster commands. Should not spring data redis for jedis also shuffle the nodes obtained from
cluster.getClusterNodes().entrySet()? Refer - https://github.com/spring-projects/spring-data-redis/blob/master/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java#L4178 -
Can we make 100ms configurable?
if (cached != null && time + 100 > System.currentTimeMillis()) -
Can we have spring-redis with jedis use BinaryJedisCluster (after it exposes
psetex(byte[], long, byte[])) to avoid topology refresh as discussed in jedis cluster & ping commands are concentrated on a single host redis/jedis#1456
Issue Links:
- DATAREDIS-749 Upgrade to Jedis 3.0
("depends on") - DATAREDIS-794 Improve JedisClusterConnection topology caching
Referenced from: pull request #463
2 votes, 4 watchers