GEODE-9922: Move Redis cross-slot checking to RegionProvider (#7295)
* GEODE-9922: Move Redis cross-slot checking to RegionProvider
- Move duplicated logic for determining if Keys are in different slots
from various Executors to RegionProvider
- Removed manual checks for if the key is local, as this is performed
as part of locking the primary bucket
- Created RedisCrossSlotException class
- Added unit tests for new method in RegionProvider
- Refactor SetOpExecutor to also lock the destination key for *STORE
commands
- Add missing test cases for cross-slot errors
- Correct some tests for cross-slot behaviour that were inadvertantly
testing the Jedis client's response rather than the Geode for Redis
server
- Changed name format for constants in AbstractSMoveIntegrationTest
- Modify patch file to ensure tcl tests use keys with the same slot
Authored-by: Donal Evans <doevans@vmware.com>
diff --git a/geode-for-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch b/geode-for-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
index 88e1318..f9952bd 100644
--- a/geode-for-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
+++ b/geode-for-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
@@ -111,7 +111,7 @@
test {Once AUTH succeeded we can actually send commands to the server} {
diff --git a/tests/unit/dump.tcl b/tests/unit/dump.tcl
-index 4c4e5d075..18bb694f2 100644
+index 4c4e5d075..e465300f4 100644
--- a/tests/unit/dump.tcl
+++ b/tests/unit/dump.tcl
@@ -41,34 +41,35 @@ start_server {tags {"dump"}} {
@@ -162,7 +162,7 @@
+# assert {$idle >= 1000 && $idle <= 1010}
+# r get foo
+# } {bar}
-+#
++#
+# test {RESTORE can set LFU} {
+# r set foo bar
+# set encoded [r dump foo]
@@ -1337,7 +1337,7 @@
# The following test can only be executed if we don't use Valgrind, and if
# we are using x86_64 architecture, because:
diff --git a/tests/unit/type/set.tcl b/tests/unit/type/set.tcl
-index 7b467f1c4..21f0721c4 100644
+index 7b467f1c4..0c5ca1753 100644
--- a/tests/unit/type/set.tcl
+++ b/tests/unit/type/set.tcl
@@ -34,8 +34,8 @@ start_server {
@@ -1360,15 +1360,341 @@
assert_encoding intset myintset
assert_encoding hashtable mylargeintset
assert_encoding hashtable myhashset
-@@ -157,7 +157,7 @@ start_server {
+@@ -113,19 +113,19 @@ start_server {
+
+ foreach {type} {hashtable intset} {
+ for {set i 1} {$i <= 5} {incr i} {
+- r del [format "set%d" $i]
++ r del [format "{tag}set%d" $i]
+ }
+ for {set i 0} {$i < 200} {incr i} {
+- r sadd set1 $i
+- r sadd set2 [expr $i+195]
++ r sadd "{tag}set1" $i
++ r sadd "{tag}set2" [expr $i+195]
+ }
+ foreach i {199 195 1000 2000} {
+- r sadd set3 $i
++ r sadd "{tag}set3" $i
+ }
+ for {set i 5} {$i < 200} {incr i} {
+- r sadd set4 $i
++ r sadd "{tag}set4" $i
+ }
+- r sadd set5 0
++ r sadd "{tag}set5" 0
+
+ # To make sure the sets are encoded as the type we are testing -- also
+ # when the VM is enabled and the values may be swapped in and out
+@@ -137,87 +137,87 @@ start_server {
+ }
+
+ for {set i 1} {$i <= 5} {incr i} {
+- r sadd [format "set%d" $i] $large
++ r sadd [format "{tag}set%d" $i] $large
+ }
+
+ test "Generated sets must be encoded as $type" {
+ for {set i 1} {$i <= 5} {incr i} {
+- assert_encoding $type [format "set%d" $i]
++ assert_encoding $type [format "{tag}set%d" $i]
+ }
+ }
+
+ test "SINTER with two sets - $type" {
+- assert_equal [list 195 196 197 198 199 $large] [lsort [r sinter set1 set2]]
++ assert_equal [list 195 196 197 198 199 $large] [lsort [r sinter "{tag}set1" "{tag}set2"]]
+ }
+
+ test "SINTERSTORE with two sets - $type" {
+- r sinterstore setres set1 set2
+- assert_encoding $type setres
+- assert_equal [list 195 196 197 198 199 $large] [lsort [r smembers setres]]
++ r sinterstore "{tag}setres" "{tag}set1" "{tag}set2"
++ assert_encoding $type "{tag}setres"
++ assert_equal [list 195 196 197 198 199 $large] [lsort [r smembers "{tag}setres"]]
}
test "SINTERSTORE with two sets, after a DEBUG RELOAD - $type" {
- r debug reload
+- r sinterstore setres set1 set2
+- assert_encoding $type setres
+- assert_equal [list 195 196 197 198 199 $large] [lsort [r smembers setres]]
+ #r debug reload
- r sinterstore setres set1 set2
- assert_encoding $type setres
- assert_equal [list 195 196 197 198 199 $large] [lsort [r smembers setres]]
++ r sinterstore "{tag}setres" "{tag}set1" "{tag}set2"
++ assert_encoding $type "{tag}setres"
++ assert_equal [list 195 196 197 198 199 $large] [lsort [r smembers "{tag}setres"]]
+ }
+
+ test "SUNION with two sets - $type" {
+- set expected [lsort -uniq "[r smembers set1] [r smembers set2]"]
+- assert_equal $expected [lsort [r sunion set1 set2]]
++ set expected [lsort -uniq "[r smembers "{tag}set1"] [r smembers "{tag}set2"]"]
++ assert_equal $expected [lsort [r sunion "{tag}set1" "{tag}set2"]]
+ }
+
+ test "SUNIONSTORE with two sets - $type" {
+- r sunionstore setres set1 set2
+- assert_encoding $type setres
+- set expected [lsort -uniq "[r smembers set1] [r smembers set2]"]
+- assert_equal $expected [lsort [r smembers setres]]
++ r sunionstore "{tag}setres" "{tag}set1" "{tag}set2"
++ assert_encoding $type "{tag}setres"
++ set expected [lsort -uniq "[r smembers "{tag}set1"] [r smembers "{tag}set2"]"]
++ assert_equal $expected [lsort [r smembers "{tag}setres"]]
+ }
+
+ test "SINTER against three sets - $type" {
+- assert_equal [list 195 199 $large] [lsort [r sinter set1 set2 set3]]
++ assert_equal [list 195 199 $large] [lsort [r sinter "{tag}set1" "{tag}set2" "{tag}set3"]]
+ }
+
+ test "SINTERSTORE with three sets - $type" {
+- r sinterstore setres set1 set2 set3
+- assert_equal [list 195 199 $large] [lsort [r smembers setres]]
++ r sinterstore "{tag}setres" "{tag}set1" "{tag}set2" "{tag}set3"
++ assert_equal [list 195 199 $large] [lsort [r smembers "{tag}setres"]]
+ }
+
+ test "SUNION with non existing keys - $type" {
+- set expected [lsort -uniq "[r smembers set1] [r smembers set2]"]
+- assert_equal $expected [lsort [r sunion nokey1 set1 set2 nokey2]]
++ set expected [lsort -uniq "[r smembers "{tag}set1"] [r smembers "{tag}set2"]"]
++ assert_equal $expected [lsort [r sunion "{tag}nokey1" "{tag}set1" "{tag}set2" "{tag}nokey2"]]
+ }
+
+ test "SDIFF with two sets - $type" {
+- assert_equal {0 1 2 3 4} [lsort [r sdiff set1 set4]]
++ assert_equal {0 1 2 3 4} [lsort [r sdiff "{tag}set1" "{tag}set4"]]
+ }
+
+ test "SDIFF with three sets - $type" {
+- assert_equal {1 2 3 4} [lsort [r sdiff set1 set4 set5]]
++ assert_equal {1 2 3 4} [lsort [r sdiff "{tag}set1" "{tag}set4" "{tag}set5"]]
+ }
+
+ test "SDIFFSTORE with three sets - $type" {
+- r sdiffstore setres set1 set4 set5
++ r sdiffstore "{tag}setres" "{tag}set1" "{tag}set4" "{tag}set5"
+ # When we start with intsets, we should always end with intsets.
+ if {$type eq {intset}} {
+- assert_encoding intset setres
++ assert_encoding intset "{tag}setres"
+ }
+- assert_equal {1 2 3 4} [lsort [r smembers setres]]
++ assert_equal {1 2 3 4} [lsort [r smembers "{tag}setres"]]
+ }
+ }
+
+ test "SDIFF with first set empty" {
+- r del set1 set2 set3
+- r sadd set2 1 2 3 4
+- r sadd set3 a b c d
+- r sdiff set1 set2 set3
++ r del "{tag}set1" "{tag}set2" "{tag}set3"
++ r sadd "{tag}set2" 1 2 3 4
++ r sadd "{tag}set3" a b c d
++ r sdiff "{tag}set1" "{tag}set2" "{tag}set3"
+ } {}
+
+ test "SDIFF with same set two times" {
+- r del set1
+- r sadd set1 a b c 1 2 3 4 5 6
+- r sdiff set1 set1
++ r del "{tag}set1"
++ r sadd "{tag}set1" a b c 1 2 3 4 5 6
++ r sdiff "{tag}set1" "{tag}set1"
+ } {}
+
+ test "SDIFF fuzzing" {
+@@ -228,11 +228,11 @@ start_server {
+ set num_sets [expr {[randomInt 10]+1}]
+ for {set i 0} {$i < $num_sets} {incr i} {
+ set num_elements [randomInt 100]
+- r del set_$i
+- lappend args set_$i
++ r del [format "{tag}set%d" $i]
++ lappend args [format "{tag}set%d" $i]
+ while {$num_elements} {
+ set ele [randomValue]
+- r sadd set_$i $ele
++ r sadd [format "{tag}set%d" $i] $ele
+ if {$i == 0} {
+ set s($ele) x
+ } else {
+@@ -247,42 +247,42 @@ start_server {
+ }
+
+ test "SINTER against non-set should throw error" {
+- r set key1 x
+- assert_error "WRONGTYPE*" {r sinter key1 noset}
++ r set "{tag}key1" x
++ assert_error "WRONGTYPE*" {r sinter "{tag}key1" "{tag}noset"}
+ }
+
+ test "SUNION against non-set should throw error" {
+- r set key1 x
+- assert_error "WRONGTYPE*" {r sunion key1 noset}
++ r set "{tag}key1" x
++ assert_error "WRONGTYPE*" {r sunion "{tag}key1" "{tag}noset"}
+ }
+
+ test "SINTER should handle non existing key as empty" {
+- r del set1 set2 set3
+- r sadd set1 a b c
+- r sadd set2 b c d
+- r sinter set1 set2 set3
++ r del "{tag}set1" "{tag}set2" "{tag}set3"
++ r sadd "{tag}set1" a b c
++ r sadd "{tag}set2" b c d
++ r sinter "{tag}set1" "{tag}set2" "{tag}set3"
+ } {}
+
+ test "SINTER with same integer elements but different encoding" {
+- r del set1 set2
+- r sadd set1 1 2 3
+- r sadd set2 1 2 3 a
+- r srem set2 a
+- assert_encoding intset set1
+- assert_encoding hashtable set2
+- lsort [r sinter set1 set2]
++ r del "{tag}set1" "{tag}set2"
++ r sadd "{tag}set1" 1 2 3
++ r sadd "{tag}set2" 1 2 3 a
++ r srem "{tag}set2" a
++ assert_encoding intset "{tag}set1"
++ assert_encoding hashtable "{tag}set2"
++ lsort [r sinter "{tag}set1" "{tag}set2"]
+ } {1 2 3}
+
+ test "SINTERSTORE against non existing keys should delete dstkey" {
+- r set setres xxx
+- assert_equal 0 [r sinterstore setres foo111 bar222]
+- assert_equal 0 [r exists setres]
++ r set "{tag}setres" xxx
++ assert_equal 0 [r sinterstore "{tag}setres" "{tag}foo111" "{tag}bar222"]
++ assert_equal 0 [r exists "{tag}setres"]
+ }
+
+ test "SUNIONSTORE against non existing keys should delete dstkey" {
+- r set setres xxx
+- assert_equal 0 [r sunionstore setres foo111 bar222]
+- assert_equal 0 [r exists setres]
++ r set "{tag}setres" xxx
++ assert_equal 0 [r sunionstore "{tag}setres" "{tag}foo111" "{tag}bar222"]
++ assert_equal 0 [r exists "{tag}setres"]
+ }
+
+ foreach {type contents} {hashtable {a b c} intset {1 2 3}} {
+@@ -486,74 +486,74 @@ start_server {
+ }
+
+ proc setup_move {} {
+- r del myset3 myset4
+- create_set myset1 {1 a b}
+- create_set myset2 {2 3 4}
+- assert_encoding hashtable myset1
+- assert_encoding intset myset2
++ r del "{tag}myset3" "{tag}myset4"
++ create_set "{tag}myset1" {1 a b}
++ create_set "{tag}myset2" {2 3 4}
++ assert_encoding hashtable "{tag}myset1"
++ assert_encoding intset "{tag}myset2"
+ }
+
+ test "SMOVE basics - from regular set to intset" {
+ # move a non-integer element to an intset should convert encoding
+ setup_move
+- assert_equal 1 [r smove myset1 myset2 a]
+- assert_equal {1 b} [lsort [r smembers myset1]]
+- assert_equal {2 3 4 a} [lsort [r smembers myset2]]
+- assert_encoding hashtable myset2
++ assert_equal 1 [r smove "{tag}myset1" "{tag}myset2" a]
++ assert_equal {1 b} [lsort [r smembers "{tag}myset1"]]
++ assert_equal {2 3 4 a} [lsort [r smembers "{tag}myset2"]]
++ assert_encoding hashtable "{tag}myset2"
+
+ # move an integer element should not convert the encoding
+ setup_move
+- assert_equal 1 [r smove myset1 myset2 1]
+- assert_equal {a b} [lsort [r smembers myset1]]
+- assert_equal {1 2 3 4} [lsort [r smembers myset2]]
+- assert_encoding intset myset2
++ assert_equal 1 [r smove "{tag}myset1" "{tag}myset2" 1]
++ assert_equal {a b} [lsort [r smembers "{tag}myset1"]]
++ assert_equal {1 2 3 4} [lsort [r smembers "{tag}myset2"]]
++ assert_encoding intset "{tag}myset2"
+ }
+
+ test "SMOVE basics - from intset to regular set" {
+ setup_move
+- assert_equal 1 [r smove myset2 myset1 2]
+- assert_equal {1 2 a b} [lsort [r smembers myset1]]
+- assert_equal {3 4} [lsort [r smembers myset2]]
++ assert_equal 1 [r smove "{tag}myset2" "{tag}myset1" 2]
++ assert_equal {1 2 a b} [lsort [r smembers "{tag}myset1"]]
++ assert_equal {3 4} [lsort [r smembers "{tag}myset2"]]
+ }
+
+ test "SMOVE non existing key" {
+ setup_move
+- assert_equal 0 [r smove myset1 myset2 foo]
+- assert_equal 0 [r smove myset1 myset1 foo]
+- assert_equal {1 a b} [lsort [r smembers myset1]]
+- assert_equal {2 3 4} [lsort [r smembers myset2]]
++ assert_equal 0 [r smove "{tag}myset1" "{tag}myset2" foo]
++ assert_equal 0 [r smove "{tag}myset1" "{tag}myset1" foo]
++ assert_equal {1 a b} [lsort [r smembers "{tag}myset1"]]
++ assert_equal {2 3 4} [lsort [r smembers "{tag}myset2"]]
+ }
+
+ test "SMOVE non existing src set" {
+ setup_move
+- assert_equal 0 [r smove noset myset2 foo]
+- assert_equal {2 3 4} [lsort [r smembers myset2]]
++ assert_equal 0 [r smove "{tag}noset" "{tag}myset2" foo]
++ assert_equal {2 3 4} [lsort [r smembers "{tag}myset2"]]
+ }
+
+ test "SMOVE from regular set to non existing destination set" {
+ setup_move
+- assert_equal 1 [r smove myset1 myset3 a]
+- assert_equal {1 b} [lsort [r smembers myset1]]
+- assert_equal {a} [lsort [r smembers myset3]]
+- assert_encoding hashtable myset3
++ assert_equal 1 [r smove "{tag}myset1" "{tag}myset3" a]
++ assert_equal {1 b} [lsort [r smembers "{tag}myset1"]]
++ assert_equal {a} [lsort [r smembers "{tag}myset3"]]
++ assert_encoding hashtable "{tag}myset3"
+ }
+
+ test "SMOVE from intset to non existing destination set" {
+ setup_move
+- assert_equal 1 [r smove myset2 myset3 2]
+- assert_equal {3 4} [lsort [r smembers myset2]]
+- assert_equal {2} [lsort [r smembers myset3]]
+- assert_encoding intset myset3
++ assert_equal 1 [r smove "{tag}myset2" "{tag}myset3" 2]
++ assert_equal {3 4} [lsort [r smembers "{tag}myset2"]]
++ assert_equal {2} [lsort [r smembers "{tag}myset3"]]
++ assert_encoding intset "{tag}myset3"
+ }
+
+ test "SMOVE wrong src key type" {
+- r set x 10
+- assert_error "WRONGTYPE*" {r smove x myset2 foo}
++ r set "{tag}x" 10
++ assert_error "WRONGTYPE*" {r smove "{tag}x" "{tag}myset2" foo}
+ }
+
+ test "SMOVE wrong dst key type" {
+- r set x 10
+- assert_error "WRONGTYPE*" {r smove myset2 x foo}
++ r set "{tag}x" 10
++ assert_error "WRONGTYPE*" {r smove "{tag}myset2" "{tag}x" foo}
+ }
+
+ test "SMOVE with identical source and destination" {
diff --git a/tests/unit/type/string.tcl b/tests/unit/type/string.tcl
index 7122fd987..2274c82cc 100644
--- a/tests/unit/type/string.tcl
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameIntegrationTest.java
index 1feb6fb..a2af1b8 100644
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameIntegrationTest.java
@@ -17,10 +17,12 @@
import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
import static org.junit.Assert.fail;
+import static redis.clients.jedis.Protocol.Command.RENAME;
import java.util.ArrayList;
import java.util.Arrays;
@@ -40,7 +42,6 @@
import org.junit.Test;
import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.JedisCluster;
-import redis.clients.jedis.Protocol;
import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
@@ -67,7 +68,17 @@
@Test
public void errors_GivenWrongNumberOfArguments() {
- assertExactNumberOfArgs(jedis, Protocol.Command.RENAME, 2);
+ assertExactNumberOfArgs(jedis, RENAME, 2);
+ }
+
+ @Test
+ public void shouldReturnCrossSlotError_givenKeysInDifferentSlots() {
+ String key1 = "{tag1}key1";
+ String key2 = "{tag2}key2";
+ jedis.set(key1, "value1");
+ jedis.set(key2, "value1");
+ assertThatThrownBy(() -> jedis.sendCommand(key1, RENAME, key1, key2))
+ .hasMessageContaining("CROSSSLOT " + ERROR_WRONG_SLOT);
}
@Test
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameNXIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameNXIntegrationTest.java
index 954cebb..04b27ac 100644
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameNXIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameNXIntegrationTest.java
@@ -17,10 +17,12 @@
import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
import static org.junit.Assert.fail;
+import static redis.clients.jedis.Protocol.Command.RENAMENX;
import java.util.ArrayList;
import java.util.Arrays;
@@ -71,6 +73,16 @@
}
@Test
+ public void shouldReturnCrossSlotError_givenKeysInDifferentSlots() {
+ String key1 = "{tag1}key1";
+ String key2 = "{tag2}key2";
+ jedis.set(key1, "value1");
+ jedis.set(key2, "value1");
+ assertThatThrownBy(() -> jedis.sendCommand(key1, RENAMENX, key1, key2))
+ .hasMessageContaining("CROSSSLOT " + ERROR_WRONG_SLOT);
+ }
+
+ @Test
public void shouldRename_givenNewKey() {
jedis.set("{tag1}foo", "bar");
long result = jedis.renamenx("{tag1}foo", "{tag1}newfoo");
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
index 3b4ec8c..7b7f7e3 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
@@ -15,11 +15,13 @@
package org.apache.geode.redis.internal.commands.executor.set;
import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static redis.clients.jedis.Protocol.Command.SDIFF;
import java.util.Arrays;
import java.util.HashSet;
@@ -31,7 +33,6 @@
import org.junit.Test;
import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.JedisCluster;
-import redis.clients.jedis.Protocol;
import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
@@ -54,7 +55,7 @@
@Test
public void sdiffErrors_givenTooFewArguments() {
- assertAtLeastNArgs(jedis, Protocol.Command.SDIFF, 1);
+ assertAtLeastNArgs(jedis, SDIFF, 1);
}
@Test
@@ -64,6 +65,16 @@
}
@Test
+ public void sdif_withSetsFromDifferentSlots_returnsCrossSlotError() {
+ String setKeyDifferentSlot = "{tag2}set2";
+ jedis.sadd(SET_KEY, "member1");
+ jedis.sadd(setKeyDifferentSlot, "member2");
+
+ assertThatThrownBy(() -> jedis.sendCommand(SET_KEY, SDIFF, SET_KEY, setKeyDifferentSlot))
+ .hasMessage("CROSSSLOT " + ERROR_WRONG_SLOT);
+ }
+
+ @Test
public void sdiffWithNonExistentSet_returnsEmptySet() {
assertThat(jedis.sdiff(NON_EXISTENT_SET_KEY)).isEmpty();
}
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffStoreIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffStoreIntegrationTest.java
index e34c3dd..c60fe6e 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffStoreIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffStoreIntegrationTest.java
@@ -15,11 +15,13 @@
package org.apache.geode.redis.internal.commands.executor.set;
import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static redis.clients.jedis.Protocol.Command.SDIFFSTORE;
import java.util.concurrent.atomic.AtomicLong;
@@ -28,7 +30,6 @@
import org.junit.Test;
import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.JedisCluster;
-import redis.clients.jedis.Protocol;
import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
@@ -56,7 +57,17 @@
@Test
public void sdiffstoreTooFewArguments_returnsError() {
- assertAtLeastNArgs(jedis, Protocol.Command.SDIFFSTORE, 2);
+ assertAtLeastNArgs(jedis, SDIFFSTORE, 2);
+ }
+
+ @Test
+ public void sdifstore_withSetsFromDifferentSlots_returnsCrossSlotError() {
+ String setKeyDifferentSlot = "{tag2}set2";
+ jedis.sadd(SET_KEY, "member1");
+ jedis.sadd(setKeyDifferentSlot, "member2");
+
+ assertThatThrownBy(() -> jedis.sendCommand(SET_KEY, SDIFFSTORE, SET_KEY, setKeyDifferentSlot))
+ .hasMessage("CROSSSLOT " + ERROR_WRONG_SLOT);
}
@Test
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
index 168c24f..115317d 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
@@ -15,9 +15,11 @@
package org.apache.geode.redis.internal.commands.executor.set;
import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static redis.clients.jedis.Protocol.Command.SINTER;
import java.util.ArrayList;
import java.util.HashSet;
@@ -57,7 +59,7 @@
@Test
public void sinterErrors_givenTooFewArguments() {
- assertAtLeastNArgs(jedis, Protocol.Command.SINTER, 1);
+ assertAtLeastNArgs(jedis, SINTER, 1);
}
@Test
@@ -66,6 +68,16 @@
}
@Test
+ public void sinter_withSetsFromDifferentSlots_returnsCrossSlotError() {
+ String setKeyDifferentSlot = "{tag2}set2";
+ jedis.sadd(SET1, "member1");
+ jedis.sadd(setKeyDifferentSlot, "member2");
+
+ assertThatThrownBy(() -> jedis.sendCommand(SET1, SINTER, SET1, setKeyDifferentSlot))
+ .hasMessage("CROSSSLOT " + ERROR_WRONG_SLOT);
+ }
+
+ @Test
public void testSInter_givenIntersection_returnsIntersectedMembers() {
String[] firstSet = new String[] {"peach"};
String[] secondSet = new String[] {"linux", "apple", "peach"};
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSMoveIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSMoveIntegrationTest.java
index 2ca43e3..ec01096 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSMoveIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSMoveIntegrationTest.java
@@ -15,11 +15,12 @@
package org.apache.geode.redis.internal.commands.executor.set;
import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_DIFFERENT_SLOTS;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static redis.clients.jedis.Protocol.Command.SMOVE;
import java.util.concurrent.atomic.AtomicLong;
@@ -29,7 +30,6 @@
import org.junit.Test;
import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.JedisCluster;
-import redis.clients.jedis.Protocol;
import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
@@ -37,12 +37,12 @@
public abstract class AbstractSMoveIntegrationTest implements RedisIntegrationTest {
private JedisCluster jedis;
- private static final String nonExistentSetKey = "{tag1}nonExistentSet";
- private static final String sourceKey = "{tag1}sourceKey";
- private static final String[] sourceMembers = {"one", "two", "three", "four", "five"};
- private static final String destKey = "{tag1}destKey";
- private static final String[] destMembers = {"a", "b", "c"};
- private static final String movedMember = "one";
+ private static final String NON_EXISTENT_SET_KEY = "{tag1}nonExistentSet";
+ private static final String SOURCE_KEY = "{tag1}sourceKey";
+ private static final String[] SOURCE_MEMBERS = {"one", "two", "three", "four", "five"};
+ private static final String DESTINATION_KEY = "{tag1}destKey";
+ private static final String[] DESTINATION_MEMBERS = {"a", "b", "c"};
+ private static final String MOVED_MEMBER = "one";
@Before
public void setUp() {
@@ -57,182 +57,187 @@
@Test
public void smove_givenWrongNumberOfArguments_returnsError() {
- assertExactNumberOfArgs(jedis, Protocol.Command.SMOVE, 3);
+ assertExactNumberOfArgs(jedis, SMOVE, 3);
}
@Test
public void smove_withWrongTypeSource_returnsWrongTypeError() {
- jedis.set(sourceKey, "value");
- jedis.sadd(destKey, destMembers);
+ jedis.set(SOURCE_KEY, "value");
+ jedis.sadd(DESTINATION_KEY, DESTINATION_MEMBERS);
- assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ assertThatThrownBy(() -> jedis.smove(SOURCE_KEY, DESTINATION_KEY, MOVED_MEMBER))
.hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
}
@Test
public void smove_withWrongTypeDest_returnsWrongTypeError() {
- jedis.sadd(sourceKey, sourceMembers);
- jedis.set(destKey, "value");
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
+ jedis.set(DESTINATION_KEY, "value");
- assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ assertThatThrownBy(() -> jedis.smove(SOURCE_KEY, DESTINATION_KEY, MOVED_MEMBER))
.hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
}
@Test
public void smove_withWrongTypeSourceAndDest_returnsWrongTypeError() {
- jedis.set(sourceKey, "sourceMember");
- jedis.set(destKey, "destMember");
+ jedis.set(SOURCE_KEY, "sourceMember");
+ jedis.set(DESTINATION_KEY, "destMember");
- assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ assertThatThrownBy(() -> jedis.smove(SOURCE_KEY, DESTINATION_KEY, MOVED_MEMBER))
.hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
}
@Test
public void smove_withNonExistentSource_returnsZero_sourceKeyDoesNotExist() {
- jedis.sadd(destKey, destMembers);
+ jedis.sadd(DESTINATION_KEY, DESTINATION_MEMBERS);
- assertThat(jedis.smove(nonExistentSetKey, destKey, movedMember))
+ assertThat(jedis.smove(NON_EXISTENT_SET_KEY, DESTINATION_KEY, MOVED_MEMBER))
.isEqualTo(0);
- assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+ assertThat(jedis.exists(NON_EXISTENT_SET_KEY)).isFalse();
}
@Test
public void smove_withNonExistentMemberInSource_returnsZero_memberNotAddedToDest() {
String nonExistentMember = "foo";
- jedis.sadd(sourceKey, sourceMembers);
- jedis.sadd(destKey, destMembers);
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
+ jedis.sadd(DESTINATION_KEY, DESTINATION_MEMBERS);
- assertThat(jedis.smove(nonExistentSetKey, destKey, nonExistentMember))
+ assertThat(jedis.smove(NON_EXISTENT_SET_KEY, DESTINATION_KEY, nonExistentMember))
.isEqualTo(0);
- assertThat(jedis.sismember(destKey, nonExistentMember)).isFalse();
+ assertThat(jedis.sismember(DESTINATION_KEY, nonExistentMember)).isFalse();
}
@Test
public void smove_withExistentSourceAndNonExistentDest_returnsOne_memberMovedFromSourceToCreatedDest() {
- jedis.sadd(sourceKey, sourceMembers);
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
- String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
- String[] destResult = new String[] {movedMember};
+ String[] sourceResult = ArrayUtils.remove(SOURCE_MEMBERS, 0);
+ String[] destResult = new String[] {MOVED_MEMBER};
- assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ assertThat(jedis.smove(SOURCE_KEY, DESTINATION_KEY, MOVED_MEMBER))
.isEqualTo(1);
- assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
- assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destResult);
+ assertThat(jedis.smembers(SOURCE_KEY)).containsExactlyInAnyOrder(sourceResult);
+ assertThat(jedis.smembers(DESTINATION_KEY)).containsExactlyInAnyOrder(destResult);
}
@Test
public void smove_withExistentSourceAndDest_returnsOne_memberMovedFromSourceToDest() {
- jedis.sadd(sourceKey, sourceMembers);
- jedis.sadd(destKey, destMembers);
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
+ jedis.sadd(DESTINATION_KEY, DESTINATION_MEMBERS);
- String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
- String[] destResult = ArrayUtils.add(destMembers, movedMember);
+ String[] sourceResult = ArrayUtils.remove(SOURCE_MEMBERS, 0);
+ String[] destResult = ArrayUtils.add(DESTINATION_MEMBERS, MOVED_MEMBER);
- assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ assertThat(jedis.smove(SOURCE_KEY, DESTINATION_KEY, MOVED_MEMBER))
.isEqualTo(1);
- assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
- assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destResult);
+ assertThat(jedis.smembers(SOURCE_KEY)).containsExactlyInAnyOrder(sourceResult);
+ assertThat(jedis.smembers(DESTINATION_KEY)).containsExactlyInAnyOrder(destResult);
}
@Test
public void smove_withSameSourceAndDest_withMemberInDest_returnsOne_setNotModified() {
- jedis.sadd(sourceKey, sourceMembers);
- assertThat(jedis.smove(sourceKey, sourceKey, movedMember))
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
+ assertThat(jedis.smove(SOURCE_KEY, SOURCE_KEY, MOVED_MEMBER))
.isEqualTo(1);
- assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceMembers);
+ assertThat(jedis.smembers(SOURCE_KEY)).containsExactlyInAnyOrder(SOURCE_MEMBERS);
}
@Test
public void smove_withExistentSourceAndDest_withMemberInDest_returnsOne_memberRemovedFromSource() {
- jedis.sadd(sourceKey, sourceMembers);
- String[] newDestMembers = ArrayUtils.add(destMembers, movedMember);
- jedis.sadd(destKey, newDestMembers);
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
+ String[] newDestMembers = ArrayUtils.add(DESTINATION_MEMBERS, MOVED_MEMBER);
+ jedis.sadd(DESTINATION_KEY, newDestMembers);
- String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
+ String[] sourceResult = ArrayUtils.remove(SOURCE_MEMBERS, 0);
- assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ assertThat(jedis.smove(SOURCE_KEY, DESTINATION_KEY, MOVED_MEMBER))
.isEqualTo(1);
- assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
- assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(newDestMembers);
+ assertThat(jedis.smembers(SOURCE_KEY)).containsExactlyInAnyOrder(sourceResult);
+ assertThat(jedis.smembers(DESTINATION_KEY)).containsExactlyInAnyOrder(newDestMembers);
}
@Test
public void smoveWithSetsFromDifferentSlots_returnsCrossSlotError() {
String setKeyDifferentSlot = "{tag2}setKey2";
- jedis.sadd(sourceKey, setKeyDifferentSlot);
- jedis.sadd(sourceKey, sourceMembers);
- jedis.sadd(setKeyDifferentSlot, destMembers);
+ jedis.sadd(SOURCE_KEY, setKeyDifferentSlot);
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
+ jedis.sadd(setKeyDifferentSlot, DESTINATION_MEMBERS);
- assertThatThrownBy(() -> jedis.smove(sourceKey, setKeyDifferentSlot, movedMember))
- .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+ assertThatThrownBy(
+ () -> jedis.sendCommand(SOURCE_KEY, SMOVE, SOURCE_KEY, setKeyDifferentSlot, MOVED_MEMBER))
+ .hasMessage("CROSSSLOT " + ERROR_WRONG_SLOT);
}
@Test
public void ensureSetConsistency_whenRunningConcurrently_withSRemAndSMove() {
- String[] sourceMemberRemoved = ArrayUtils.remove(sourceMembers, 0);
- String[] destMemberAdded = ArrayUtils.add(destMembers, movedMember);
+ String[] sourceMemberRemoved = ArrayUtils.remove(SOURCE_MEMBERS, 0);
+ String[] destMemberAdded = ArrayUtils.add(DESTINATION_MEMBERS, MOVED_MEMBER);
- jedis.sadd(sourceKey, sourceMembers);
- jedis.sadd(destKey, destMembers);
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
+ jedis.sadd(DESTINATION_KEY, DESTINATION_MEMBERS);
final AtomicLong moved = new AtomicLong(0);
new ConcurrentLoopingThreads(1000,
- i -> jedis.srem(sourceKey, movedMember),
- i -> moved.set(jedis.smove(sourceKey, destKey, movedMember)))
+ i -> jedis.srem(SOURCE_KEY, MOVED_MEMBER),
+ i -> moved.set(jedis.smove(SOURCE_KEY, DESTINATION_KEY, MOVED_MEMBER)))
.runWithAction(() -> {
if (moved.get() == 1) {
- assertThat(jedis.smembers(sourceKey))
+ assertThat(jedis.smembers(SOURCE_KEY))
.containsExactlyInAnyOrder(sourceMemberRemoved);
- assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destMemberAdded);
+ assertThat(jedis.smembers(DESTINATION_KEY))
+ .containsExactlyInAnyOrder(destMemberAdded);
} else {
- assertThat(jedis.smembers(sourceKey))
+ assertThat(jedis.smembers(SOURCE_KEY))
.containsExactlyInAnyOrder(sourceMemberRemoved);
- assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destMembers);
+ assertThat(jedis.smembers(DESTINATION_KEY)).containsExactlyInAnyOrder(
+ DESTINATION_MEMBERS);
}
- jedis.sadd(sourceKey, movedMember);
- jedis.srem(destKey, movedMember);
+ jedis.sadd(SOURCE_KEY, MOVED_MEMBER);
+ jedis.srem(DESTINATION_KEY, MOVED_MEMBER);
});
}
@Test
public void ensureSetConsistency_whenRunningConcurrently_withSMovesFromSameSourceAndDifferentDestination() {
- String[] sourceMemberRemoved = ArrayUtils.remove(sourceMembers, 0);
- String[] destMemberAdded = ArrayUtils.add(destMembers, movedMember);
- String[] nonExisistentMemberAdded = {movedMember};
+ String[] sourceMemberRemoved = ArrayUtils.remove(SOURCE_MEMBERS, 0);
+ String[] destMemberAdded = ArrayUtils.add(DESTINATION_MEMBERS, MOVED_MEMBER);
+ String[] nonExisistentMemberAdded = {MOVED_MEMBER};
- jedis.sadd(sourceKey, sourceMembers);
- jedis.sadd(destKey, destMembers);
+ jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
+ jedis.sadd(DESTINATION_KEY, DESTINATION_MEMBERS);
final AtomicLong movedToNonExistent = new AtomicLong(0);
final AtomicLong movedToDest = new AtomicLong(0);
new ConcurrentLoopingThreads(1000,
- i -> movedToNonExistent.set(jedis.smove(sourceKey, nonExistentSetKey, movedMember)),
- i -> movedToDest.set(jedis.smove(sourceKey, destKey, movedMember)))
+ i -> movedToNonExistent.set(jedis.smove(SOURCE_KEY, NON_EXISTENT_SET_KEY, MOVED_MEMBER)),
+ i -> movedToDest.set(jedis.smove(SOURCE_KEY, DESTINATION_KEY, MOVED_MEMBER)))
.runWithAction(() -> {
// Asserts that only one smove was preformed
assertThat(movedToNonExistent.get() ^ movedToDest.get()).isEqualTo(1);
- assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceMemberRemoved);
+ assertThat(jedis.smembers(SOURCE_KEY)).containsExactlyInAnyOrder(sourceMemberRemoved);
if (movedToNonExistent.get() == 1) {
- assertThat(jedis.smembers(nonExistentSetKey))
+ assertThat(jedis.smembers(NON_EXISTENT_SET_KEY))
.containsExactlyInAnyOrder(nonExisistentMemberAdded);
} else {
- assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+ assertThat(jedis.exists(NON_EXISTENT_SET_KEY)).isFalse();
}
if (movedToDest.get() == 1) {
- assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destMemberAdded);
+ assertThat(jedis.smembers(DESTINATION_KEY))
+ .containsExactlyInAnyOrder(destMemberAdded);
} else {
- assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destMembers);
+ assertThat(jedis.smembers(DESTINATION_KEY)).containsExactlyInAnyOrder(
+ DESTINATION_MEMBERS);
}
- jedis.sadd(sourceKey, movedMember);
- jedis.srem(destKey, movedMember);
- jedis.srem(nonExistentSetKey, movedMember);
+ jedis.sadd(SOURCE_KEY, MOVED_MEMBER);
+ jedis.srem(DESTINATION_KEY, MOVED_MEMBER);
+ jedis.srem(NON_EXISTENT_SET_KEY, MOVED_MEMBER);
});
}
}
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
index 7e6ac15..40f0314 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
@@ -15,12 +15,13 @@
package org.apache.geode.redis.internal.commands.executor.set;
import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_DIFFERENT_SLOTS;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static redis.clients.jedis.Protocol.Command.SUNION;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
@@ -30,7 +31,6 @@
import org.junit.Test;
import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.JedisCluster;
-import redis.clients.jedis.Protocol;
import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
@@ -55,7 +55,7 @@
@Test
public void sunionErrors_givenTooFewArguments() {
- assertAtLeastNArgs(jedis, Protocol.Command.SUNION, 1);
+ assertAtLeastNArgs(jedis, SUNION, 1);
}
@Test
@@ -126,8 +126,8 @@
jedis.sadd(SET_KEY_1, SET_MEMBERS_1);
jedis.sadd(setKeyDifferentSlot, secondSetMembers);
- assertThatThrownBy(() -> jedis.sunion(SET_KEY_1, setKeyDifferentSlot))
- .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+ assertThatThrownBy(() -> jedis.sendCommand(SET_KEY_1, SUNION, SET_KEY_1, setKeyDifferentSlot))
+ .hasMessage("CROSSSLOT " + ERROR_WRONG_SLOT);
}
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionStoreIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionStoreIntegrationTest.java
index 7d0ee9b..b6526c1 100644
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionStoreIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionStoreIntegrationTest.java
@@ -15,12 +15,13 @@
package org.apache.geode.redis.internal.commands.executor.set;
import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_DIFFERENT_SLOTS;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static redis.clients.jedis.Protocol.Command.SUNIONSTORE;
import java.util.concurrent.atomic.AtomicLong;
@@ -206,8 +207,9 @@
jedis.sadd(SET_KEY_1, SET_MEMBERS_1);
jedis.sadd(setKeyDifferentSlot, secondSetMembers);
- assertThatThrownBy(() -> jedis.sunionstore(DESTINATION_KEY, SET_KEY_1, setKeyDifferentSlot))
- .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+ assertThatThrownBy(() -> jedis.sendCommand(DESTINATION_KEY, SUNIONSTORE, DESTINATION_KEY,
+ SET_KEY_1, setKeyDifferentSlot))
+ .hasMessage("CROSSSLOT " + ERROR_WRONG_SLOT);
}
@Test
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetIntegrationTest.java
index 806bbad..65462f3 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetIntegrationTest.java
@@ -14,6 +14,7 @@
*/
package org.apache.geode.redis.internal.commands.executor.string;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
@@ -71,7 +72,7 @@
public void givenDifferentSlots_returnsError() {
assertThatThrownBy(
() -> jedis.sendCommand("key1", Protocol.Command.MSET, "key1", "value1", "key2", "value2"))
- .hasMessageContaining("CROSSSLOT Keys in request don't hash to the same slot");
+ .hasMessageContaining("CROSSSLOT " + ERROR_WRONG_SLOT);
}
@Test
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetNXIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetNXIntegrationTest.java
index 2aa30d4..96f7ae5 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetNXIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetNXIntegrationTest.java
@@ -14,6 +14,7 @@
*/
package org.apache.geode.redis.internal.commands.executor.string;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
@@ -73,7 +74,7 @@
assertThatThrownBy(
() -> jedis.sendCommand("key1", Protocol.Command.MSETNX, "key1", "value1", "key2",
"value2"))
- .hasMessageContaining("CROSSSLOT Keys in request don't hash to the same slot");
+ .hasMessageContaining("CROSSSLOT " + ERROR_WRONG_SLOT);
}
@Test
diff --git a/geode-for-redis/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt b/geode-for-redis/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt
index b0aec9c..3d87db1 100644
--- a/geode-for-redis/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt
+++ b/geode-for-redis/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt
@@ -2,6 +2,7 @@
org/apache/geode/redis/internal/data/collections/Bytes2ObjectOpenHashMap
org/apache/geode/redis/internal/data/collections/SizeableBytes2ObjectOpenCustomHashMapWithCursor
org/apache/geode/redis/internal/data/collections/SizeableObjectOpenCustomHashSet
+org/apache/geode/redis/internal/data/RedisCrossSlotException
org/apache/geode/redis/internal/data/RedisDataMovedException
org/apache/geode/redis/internal/data/RedisDataTypeMismatchException
org/apache/geode/redis/internal/commands/executor/sortedset/ZAggregator
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
index 25894a3..3a0d4fe 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
@@ -78,8 +78,6 @@
"DUMP payload version or checksum are wrong";
public static final String ERROR_WRONG_SLOT =
"Keys in request don't hash to the same slot";
- public static final String ERROR_DIFFERENT_SLOTS =
- "No way to dispatch this command to Redis Cluster because keys have different slots.";
public static final String ERROR_WEIGHT_NOT_A_FLOAT =
"weight value is not a float";
public static final String ERROR_INVALID_USERNAME_OR_PASSWORD =
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameExecutor.java
index 62c5a47..d022e20 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/key/AbstractRenameExecutor.java
@@ -16,7 +16,6 @@
package org.apache.geode.redis.internal.commands.executor.key;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import java.util.Arrays;
import java.util.List;
@@ -41,10 +40,6 @@
return getTargetSameAsSourceResponse();
}
- if (key.getSlot() != newKey.getSlot()) {
- return RedisResponse.crossSlot(ERROR_WRONG_SLOT);
- }
-
try {
if (!executeRenameCommand(key, newKey, context)) {
return getNoSuchKeyResponse();
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SMoveExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SMoveExecutor.java
index 2a9b4e4..42647ba 100755
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SMoveExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SMoveExecutor.java
@@ -14,7 +14,6 @@
*/
package org.apache.geode.redis.internal.commands.executor.set;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.redis.internal.data.RedisSet.smove;
import java.util.Arrays;
@@ -23,7 +22,6 @@
import org.apache.geode.redis.internal.commands.Command;
import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
import org.apache.geode.redis.internal.commands.executor.RedisResponse;
-import org.apache.geode.redis.internal.data.RedisDataMovedException;
import org.apache.geode.redis.internal.data.RedisKey;
import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
import org.apache.geode.redis.internal.services.RegionProvider;
@@ -38,13 +36,6 @@
byte[] member = commandElems.get(3);
RegionProvider regionProvider = context.getRegionProvider();
- try {
- regionProvider.ensureKeyIsLocal(sourceKey);
- regionProvider.ensureKeyIsLocal(destKey);
- } catch (RedisDataMovedException ex) {
- return RedisResponse.crossSlot(ERROR_WRONG_SLOT);
- }
-
int removed = context.lockedExecute(sourceKey, Arrays.asList(sourceKey, destKey),
() -> smove(sourceKey, destKey, member, regionProvider));
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
index b83c8e2..9f6fce0 100755
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
@@ -29,7 +29,6 @@
import org.apache.geode.redis.internal.commands.RedisCommandType;
import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
import org.apache.geode.redis.internal.commands.executor.RedisResponse;
-import org.apache.geode.redis.internal.data.RedisDataMovedException;
import org.apache.geode.redis.internal.data.RedisKey;
import org.apache.geode.redis.internal.data.RedisSet;
import org.apache.geode.redis.internal.data.RedisSet.MemberSet;
@@ -49,13 +48,6 @@
List<RedisKey> commandElements = command.getProcessedCommandKeys();
List<RedisKey> setKeys = commandElements.subList(setsStartIndex, commandElements.size());
RegionProvider regionProvider = context.getRegionProvider();
- try {
- for (RedisKey k : setKeys) {
- regionProvider.ensureKeyIsLocal(k);
- }
- } catch (RedisDataMovedException ex) {
- return RedisResponse.error(ex.getMessage());
- }
/*
* SINTERSTORE currently use the else part of the code for their implementation.
@@ -63,20 +55,24 @@
* Refactor so the implementation is in the executor. After delete doActualSetOperation,
* doStoreSetOp, doStoreSetOpWhileLocked, computeStoreSetOp, fetchSets
*/
+ List<RedisKey> keysToLock = new ArrayList<>(setKeys);
+ if (isStorage()) {
+ keysToLock.add(command.getKey());
+ }
if (command.isOfType(RedisCommandType.SDIFF) || command.isOfType(RedisCommandType.SDIFFSTORE)) {
if (isStorage()) {
RedisKey destinationKey = command.getKey();
- int resultSize = context.lockedExecute(destinationKey, new ArrayList<>(setKeys),
+ int resultSize = context.lockedExecute(destinationKey, keysToLock,
() -> sdiffstore(regionProvider, destinationKey, setKeys));
return RedisResponse.integer(resultSize);
}
- Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), new ArrayList<>(setKeys),
+ Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), keysToLock,
() -> sdiff(regionProvider, setKeys));
return RedisResponse.array(resultSet, true);
} else if (command.isOfType(RedisCommandType.SINTER)) {
- Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), new ArrayList<>(setKeys),
+ Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), keysToLock,
() -> sinter(regionProvider, setKeys));
return RedisResponse.array(resultSet, true);
@@ -84,12 +80,12 @@
|| command.isOfType(RedisCommandType.SUNIONSTORE)) {
if (isStorage()) {
RedisKey destinationKey = command.getKey();
- int resultSize = context.lockedExecute(destinationKey, new ArrayList<>(setKeys),
+ int resultSize = context.lockedExecute(destinationKey, keysToLock,
() -> sunionstore(regionProvider, destinationKey, setKeys));
return RedisResponse.integer(resultSize);
}
- Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), new ArrayList<>(setKeys),
+ Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), keysToLock,
() -> sunion(regionProvider, setKeys));
return RedisResponse.array(resultSet, true);
}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZInterStoreExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZInterStoreExecutor.java
index 0ddff26..64bbc6a 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZInterStoreExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZInterStoreExecutor.java
@@ -32,7 +32,7 @@
List<ZKeyWeight> keyWeights, ZAggregator aggregator) {
RegionProvider regionProvider = context.getRegionProvider();
RedisKey key = command.getKey();
- List<RedisKey> keysToLock = getKeysToLock(regionProvider, key, keyWeights);
+ List<RedisKey> keysToLock = getKeysToLock(key, keyWeights);
return context.lockedExecute(key, keysToLock,
() -> zinterstore(regionProvider, key, keyWeights, aggregator));
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZStoreExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZStoreExecutor.java
index c9aa5d7..335a35e 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZStoreExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZStoreExecutor.java
@@ -17,7 +17,6 @@
import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_WEIGHT_NOT_A_FLOAT;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
import static org.apache.geode.redis.internal.netty.Coder.toUpperCaseBytes;
import static org.apache.geode.redis.internal.netty.StringBytesGlossary.AGGREGATE;
@@ -34,7 +33,6 @@
import org.apache.geode.redis.internal.data.RedisKey;
import org.apache.geode.redis.internal.netty.Coder;
import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
-import org.apache.geode.redis.internal.services.RegionProvider;
public abstract class ZStoreExecutor implements CommandExecutor {
@@ -105,24 +103,14 @@
}
}
- int slot = command.getKey().getSlot();
- for (ZKeyWeight keyWeight : keyWeights) {
- if (keyWeight.getKey().getSlot() != slot) {
- return RedisResponse.crossSlot(ERROR_WRONG_SLOT);
- }
- }
-
return RedisResponse.integer(getResult(context, command, keyWeights, aggregator));
}
- protected List<RedisKey> getKeysToLock(RegionProvider regionProvider, RedisKey destinationKey,
- List<ZKeyWeight> keyWeights) {
+ protected List<RedisKey> getKeysToLock(RedisKey destinationKey, List<ZKeyWeight> keyWeights) {
List<RedisKey> keysToLock = new ArrayList<>(keyWeights.size());
for (ZKeyWeight kw : keyWeights) {
- regionProvider.ensureKeyIsLocal(kw.getKey());
keysToLock.add(kw.getKey());
}
- regionProvider.ensureKeyIsLocal(destinationKey);
keysToLock.add(destinationKey);
return keysToLock;
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZUnionStoreExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZUnionStoreExecutor.java
index 6d30155..dee9f1a 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZUnionStoreExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/sortedset/ZUnionStoreExecutor.java
@@ -31,7 +31,7 @@
List<ZKeyWeight> keyWeights, ZAggregator aggregator) {
RegionProvider regionProvider = context.getRegionProvider();
RedisKey key = command.getKey();
- List<RedisKey> keysToLock = getKeysToLock(regionProvider, key, keyWeights);
+ List<RedisKey> keysToLock = getKeysToLock(key, keyWeights);
return context.lockedExecute(key, keysToLock,
() -> zunionstore(regionProvider, key, keyWeights, aggregator));
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetExecutor.java
index 4d7c4da..e23db4b 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/string/AbstractMSetExecutor.java
@@ -66,15 +66,9 @@
protected void mset(ExecutionHandlerContext context, List<RedisKey> keys, List<byte[]> values,
boolean nx) {
- List<RedisKey> keysToLock = new ArrayList<>(keys.size());
+ List<RedisKey> keysToLock = new ArrayList<>(keys);
RegionProvider regionProvider = context.getRegionProvider();
- for (RedisKey key : keys) {
- regionProvider.ensureKeyIsLocal(key);
- keysToLock.add(key);
- }
- // Pass a key in so that the bucket will be locked. Since all keys are already guaranteed to be
- // in the same bucket we can use any key for this.
context.lockedExecuteInTransaction(keysToLock.get(0), keysToLock,
() -> mset0(regionProvider, keys, values, nx));
}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisCrossSlotException.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisCrossSlotException.java
new file mode 100644
index 0000000..e05d46b
--- /dev/null
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisCrossSlotException.java
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.data;
+
+import org.apache.geode.redis.internal.RedisException;
+
+public class RedisCrossSlotException extends RedisException {
+
+ private static final long serialVersionUID = -2126545465506588852L;
+
+ public RedisCrossSlotException() {
+ super();
+ }
+
+ public RedisCrossSlotException(String message) {
+ super(message);
+ }
+}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
index bd8fc03..c375851 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
@@ -51,6 +51,7 @@
import org.apache.geode.redis.internal.commands.executor.RedisResponse;
import org.apache.geode.redis.internal.commands.executor.UnknownExecutor;
import org.apache.geode.redis.internal.commands.parameters.RedisParametersMismatchException;
+import org.apache.geode.redis.internal.data.RedisCrossSlotException;
import org.apache.geode.redis.internal.data.RedisData;
import org.apache.geode.redis.internal.data.RedisDataMovedException;
import org.apache.geode.redis.internal.data.RedisDataType;
@@ -182,6 +183,8 @@
return RedisResponse.moved(rootCause.getMessage());
} else if (rootCause instanceof RedisDataTypeMismatchException) {
return RedisResponse.wrongType(rootCause.getMessage());
+ } else if (rootCause instanceof RedisCrossSlotException) {
+ return RedisResponse.crossSlot(rootCause.getMessage());
} else if (rootCause instanceof IllegalStateException
|| rootCause instanceof RedisParametersMismatchException
|| rootCause instanceof RedisException
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/RegionProvider.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/RegionProvider.java
index 0f8fdd2..55e37e8 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/RegionProvider.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/RegionProvider.java
@@ -14,6 +14,7 @@
*/
package org.apache.geode.redis.internal.services;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
import static org.apache.geode.redis.internal.RedisProperties.REDIS_REGION_NAME_PROPERTY;
import static org.apache.geode.redis.internal.RedisProperties.REGION_BUCKETS;
import static org.apache.geode.redis.internal.RedisProperties.getIntegerSystemProperty;
@@ -27,6 +28,7 @@
import java.util.Set;
import java.util.concurrent.Callable;
+import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.cache.CacheTransactionManager;
import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
@@ -46,6 +48,7 @@
import org.apache.geode.redis.internal.RedisConstants;
import org.apache.geode.redis.internal.RedisException;
import org.apache.geode.redis.internal.commands.executor.cluster.RedisPartitionResolver;
+import org.apache.geode.redis.internal.data.RedisCrossSlotException;
import org.apache.geode.redis.internal.data.RedisData;
import org.apache.geode.redis.internal.data.RedisDataMovedException;
import org.apache.geode.redis.internal.data.RedisDataType;
@@ -63,13 +66,12 @@
* The name of the region that holds data stored in redis.
*/
public static final String DEFAULT_REDIS_REGION_NAME = "GEODE_FOR_REDIS";
- public static final String REDIS_REGION_BUCKETS_PARAM = REGION_BUCKETS;
public static final int REDIS_SLOTS = 16384;
// Ideally the bucket count should be a power of 2, but technically it is not required.
public static final int REDIS_REGION_BUCKETS =
- getIntegerSystemProperty(REDIS_REGION_BUCKETS_PARAM, 128, 1, REDIS_SLOTS);
+ getIntegerSystemProperty(REGION_BUCKETS, 128, 1, REDIS_SLOTS);
public static final int REDIS_SLOTS_PER_BUCKET = REDIS_SLOTS / REDIS_REGION_BUCKETS;
@@ -144,6 +146,9 @@
public <T> T lockedExecute(RedisKey key, List<RedisKey> keysToLock, Callable<T> callable) {
try {
+ if (areKeysCrossSlots(keysToLock)) {
+ throw new RedisCrossSlotException(ERROR_WRONG_SLOT);
+ }
return partitionedRegion.computeWithPrimaryLocked(key,
() -> stripedCoordinator.execute(keysToLock, callable));
} catch (PrimaryBucketLockException | BucketMovedException | RegionDestroyedException ex) {
@@ -155,6 +160,17 @@
}
}
+ @VisibleForTesting
+ static boolean areKeysCrossSlots(List<RedisKey> keysToLock) {
+ int slot = keysToLock.get(0).getSlot();
+ for (RedisKey key : keysToLock) {
+ if (key.getSlot() != slot) {
+ return true;
+ }
+ }
+ return false;
+ }
+
/**
* Execute the given Callable in the context of a GemFire transaction. On failure there is no
* attempt to retry.
diff --git a/geode-for-redis/src/test/java/org/apache/geode/redis/internal/services/RegionProviderTest.java b/geode-for-redis/src/test/java/org/apache/geode/redis/internal/services/RegionProviderTest.java
new file mode 100644
index 0000000..ea1433c
--- /dev/null
+++ b/geode-for-redis/src/test/java/org/apache/geode/redis/internal/services/RegionProviderTest.java
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.services;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.Test;
+
+import org.apache.geode.redis.internal.data.RedisKey;
+
+public class RegionProviderTest {
+
+ @Test
+ public void areKeysCrossSlotsReturnsFalseWhenKeysAreSameSlot() {
+ RedisKey key1 = mock(RedisKey.class);
+ int slot1 = 1;
+ when(key1.getSlot()).thenReturn(slot1);
+ RedisKey key2 = mock(RedisKey.class);
+ when(key2.getSlot()).thenReturn(slot1);
+
+ List<RedisKey> keyList = Arrays.asList(key1, key2);
+
+ assertThat(RegionProvider.areKeysCrossSlots(keyList)).isFalse();
+ }
+
+ @Test
+ public void areKeysCrossSlotsReturnsTrueWhenKeysAreCrossSlots() {
+ RedisKey key1 = mock(RedisKey.class);
+ int slot1 = 1;
+ when(key1.getSlot()).thenReturn(slot1);
+ RedisKey key2 = mock(RedisKey.class);
+ int slot2 = 2;
+ when(key2.getSlot()).thenReturn(slot2);
+
+ List<RedisKey> keyList = Arrays.asList(key1, key2);
+
+ assertThat(RegionProvider.areKeysCrossSlots(keyList)).isTrue();
+ }
+
+ @Test
+ public void areKeysCrossSlotsReturnsTrueWhenKeysAreCrossSlotsForManyKeys() {
+ List<RedisKey> keyList = new ArrayList<>();
+ for (int i = 0; i < 100; ++i) {
+ RedisKey key = mock(RedisKey.class);
+ int slot1 = 1;
+ when(key.getSlot()).thenReturn(slot1);
+ keyList.add(key);
+ }
+ RedisKey finalKey = mock(RedisKey.class);
+ int slot2 = 2;
+ when(finalKey.getSlot()).thenReturn(slot2);
+ keyList.add(finalKey);
+
+ assertThat(RegionProvider.areKeysCrossSlots(keyList)).isTrue();
+ }
+}
