From 672f5e49c7e6feba18a1e947a5b91128a4d2a32e Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 28 Jun 2022 11:08:03 +0200 Subject: [PATCH] Fix Cluster sort. Jedis Cluster sort now considers if the destination key is sharing the same slot as the source key to use same-slot sorting. Additionally, sort results that do not map to the same slot replace the destination key with a list instead of checking the key type and appending results. Closes #2341 --- .../jedis/JedisClusterKeyCommands.java | 27 ++++++++----------- .../lettuce/LettuceClusterKeyCommands.java | 21 +++------------ .../connection/ClusterConnectionTests.java | 4 +-- .../jedis/JedisClusterConnectionTests.java | 13 ++++++--- .../LettuceClusterConnectionTests.java | 13 ++++++--- 5 files changed, 35 insertions(+), 43 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterKeyCommands.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterKeyCommands.java index 27e1ce3f5..16788ae67 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterKeyCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterKeyCommands.java @@ -47,7 +47,6 @@ import org.springframework.data.redis.core.ScanIteration; import org.springframework.data.redis.core.ScanOptions; import org.springframework.lang.Nullable; import org.springframework.util.Assert; -import org.springframework.util.CollectionUtils; import org.springframework.util.ObjectUtils; /** @@ -541,23 +540,19 @@ class JedisClusterKeyCommands implements RedisKeyCommands { Assert.notNull(key, "Key must not be null!"); - List sorted = sort(key, params); - if (!CollectionUtils.isEmpty(sorted)) { - - byte[][] arr = new byte[sorted.size()][]; - switch (type(key)) { - - case SET: - connection.setCommands().sAdd(storeKey, sorted.toArray(arr)); - return 1L; - case LIST: - connection.listCommands().lPush(storeKey, sorted.toArray(arr)); - return 1L; - default: - throw new IllegalArgumentException("sort and store is only supported for SET and LIST"); + if (ClusterSlotHashUtil.isSameSlotForAllKeys(key, storeKey)) { + try { + return connection.getCluster().sort(key, JedisConverters.toSortingParams(params), storeKey); + } catch (Exception ex) { + throw convertJedisAccessException(ex); } } - return 0L; + + List sorted = sort(key, params); + byte[][] arr = new byte[sorted.size()][]; + connection.keyCommands().unlink(storeKey); + connection.listCommands().lPush(storeKey, sorted.toArray(arr)); + return (long) sorted.size(); } /* diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceClusterKeyCommands.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceClusterKeyCommands.java index f5a0eeb89..7c60cea31 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceClusterKeyCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceClusterKeyCommands.java @@ -33,7 +33,6 @@ import org.springframework.data.redis.core.ScanCursor; import org.springframework.data.redis.core.ScanOptions; import org.springframework.lang.Nullable; import org.springframework.util.Assert; -import org.springframework.util.CollectionUtils; /** * @author Christoph Strobl @@ -229,21 +228,9 @@ class LettuceClusterKeyCommands extends LettuceKeyCommands { } List sorted = sort(key, params); - if (!CollectionUtils.isEmpty(sorted)) { - - byte[][] arr = new byte[sorted.size()][]; - switch (type(key)) { - - case SET: - connection.setCommands().sAdd(storeKey, sorted.toArray(arr)); - return 1L; - case LIST: - connection.listCommands().lPush(storeKey, sorted.toArray(arr)); - return 1L; - default: - throw new IllegalArgumentException("sort and store is only supported for SET and LIST"); - } - } - return 0L; + byte[][] arr = new byte[sorted.size()][]; + connection.keyCommands().unlink(storeKey); + connection.listCommands().lPush(storeKey, sorted.toArray(arr)); + return (long) sorted.size(); } } diff --git a/src/test/java/org/springframework/data/redis/connection/ClusterConnectionTests.java b/src/test/java/org/springframework/data/redis/connection/ClusterConnectionTests.java index cd2488e34..5f3d88a89 100644 --- a/src/test/java/org/springframework/data/redis/connection/ClusterConnectionTests.java +++ b/src/test/java/org/springframework/data/redis/connection/ClusterConnectionTests.java @@ -568,8 +568,8 @@ public interface ClusterConnectionTests { // DATAREDIS-315 void sortAndStoreShouldAddSortedValuesValuesCorrectly(); - // DATAREDIS-315 - void sortAndStoreShouldReturnZeroWhenListDoesNotExist(); + // DATAREDIS-315, GH-2341 + void sortAndStoreShouldReplaceDestinationList(); // DATAREDIS-315 void sortShouldReturnValuesCorrectly(); 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 818884293..178a3a0ff 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 @@ -1906,13 +1906,18 @@ public class JedisClusterConnectionTests implements ClusterConnectionTests { nativeConnection.lpush(KEY_1, VALUE_2, VALUE_1); - assertThat(clusterConnection.sort(KEY_1_BYTES, new DefaultSortParameters().alpha(), KEY_2_BYTES)).isEqualTo(1L); + assertThat(clusterConnection.sort(KEY_1_BYTES, new DefaultSortParameters().alpha(), KEY_2_BYTES)).isEqualTo(2L); assertThat(nativeConnection.exists(KEY_2_BYTES)).isTrue(); } - @Test // DATAREDIS-315 - public void sortAndStoreShouldReturnZeroWhenListDoesNotExist() { - assertThat(clusterConnection.sort(KEY_1_BYTES, new DefaultSortParameters().alpha(), KEY_2_BYTES)).isEqualTo(0L); + @Test // DATAREDIS-315, GH-2341 + public void sortAndStoreShouldReplaceDestinationList() { + + nativeConnection.lpush(KEY_1, VALUE_2, VALUE_1); + nativeConnection.lpush(KEY_2_BYTES, VALUE_3_BYTES); + + assertThat(clusterConnection.sort(KEY_1_BYTES, new DefaultSortParameters().alpha(), KEY_2_BYTES)).isEqualTo(2L); + assertThat(nativeConnection.llen(KEY_2_BYTES)).isEqualTo(2); } @Test // DATAREDIS-315 diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionTests.java index 72913a826..5352030a1 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionTests.java @@ -1937,13 +1937,18 @@ public class LettuceClusterConnectionTests implements ClusterConnectionTests { nativeConnection.lpush(KEY_1, VALUE_2, VALUE_1); - assertThat(clusterConnection.sort(KEY_1_BYTES, new DefaultSortParameters().alpha(), KEY_2_BYTES)).isEqualTo(1L); + assertThat(clusterConnection.sort(KEY_1_BYTES, new DefaultSortParameters().alpha(), KEY_2_BYTES)).isEqualTo(2L); assertThat(nativeConnection.exists(KEY_2)).isEqualTo(1L); } - @Test // DATAREDIS-315 - public void sortAndStoreShouldReturnZeroWhenListDoesNotExist() { - assertThat(clusterConnection.sort(KEY_1_BYTES, new DefaultSortParameters().alpha(), KEY_2_BYTES)).isEqualTo(0L); + @Test // DATAREDIS-315, GH-2341 + public void sortAndStoreShouldReplaceDestinationList() { + + nativeConnection.lpush(KEY_1, VALUE_2, VALUE_1); + nativeConnection.lpush(KEY_2, VALUE_3); + + assertThat(clusterConnection.sort(KEY_1_BYTES, new DefaultSortParameters().alpha(), KEY_2_BYTES)).isEqualTo(2L); + assertThat(nativeConnection.llen(KEY_2)).isEqualTo(2); } @Test // DATAREDIS-315