jdeppe-pivotal commented on a change in pull request #7227:
URL: https://github.com/apache/geode/pull/7227#discussion_r777633900
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SMoveExecutor.java
##########
@@ -14,47 +14,41 @@
*/
package org.apache.geode.redis.internal.commands.executor.set;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
-import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_SET;
+import static org.apache.geode.redis.internal.data.RedisSet.smove;
import java.util.ArrayList;
-import java.util.Collections;
import java.util.List;
-import org.apache.geode.cache.Region;
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.RedisData;
+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;
public class SMoveExecutor implements CommandExecutor {
@Override
public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
List<byte[]> commandElems = command.getProcessedCommand();
- Region<RedisKey, RedisData> region = context.getRegion();
- RedisKey source = command.getKey();
- RedisKey destination = new RedisKey(commandElems.get(2));
- byte[] member = commandElems.get(3);
-
- // TODO: this command should lock both source and destination before
changing them
+ List<RedisKey> commandKeys = command.getProcessedCommandKeys();
+ List<RedisKey> setKeys = commandKeys.subList(1, 3);
- String destinationType = context.dataLockedExecute(destination, false,
RedisData::type);
- if (!destinationType.equals(REDIS_SET.toString()) &&
!destinationType.equals("none")) {
- return RedisResponse.wrongType(ERROR_WRONG_TYPE);
+ byte[] member = commandElems.get(3);
+ RegionProvider regionProvider = context.getRegionProvider();
+ try {
+ for (RedisKey k : setKeys) {
+ regionProvider.ensureKeyIsLocal(k);
+ }
+ } catch (RedisDataMovedException ex) {
Review comment:
Do you know that this is necessary? Is there a test that covers this
scenario for native Redis (I couldn't see one).
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSMoveIntegrationTest.java
##########
@@ -53,118 +55,132 @@ public void tearDown() {
}
@Test
- public void errors_GivenWrongNumberOfArguments() {
+ public void smove_givenWrongNumberOfArguments_returnsError() {
assertExactNumberOfArgs(jedis, Protocol.Command.SMOVE, 3);
}
@Test
- public void testSmove_returnsWrongType_whenWrongSourceIsUsed() {
- jedis.set("{user1}a-string", "value");
- assertThatThrownBy(() -> jedis.smove("{user1}a-string", "{user1}some-set",
"foo"))
- .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
-
- jedis.hset("{user1}a-hash", "field", "value");
- assertThatThrownBy(() -> jedis.smove("{user1}a-hash", "{user1}some-set",
"foo"))
- .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ public void smove_withWrongTypeSource_returnsWrongTypeError() {
+ jedis.set(sourceKey, "value");
+ jedis.sadd(destKey, destMembers);
+
+ assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
}
@Test
- public void testSmove_returnsWrongType_whenWrongDestinationIsUsed() {
- jedis.sadd("{user1}a-set", "foobaz");
+ public void smove_withWrongTypeDest_returnsWrongTypeError() {
+ jedis.sadd(sourceKey, sourceMembers);
+ jedis.set(destKey, "value");
- jedis.set("{user1}a-string", "value");
- assertThatThrownBy(() -> jedis.smove("{user1}a-set", "{user1}a-string",
"foo"))
- .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
+ }
- jedis.hset("{user1}a-hash", "field", "value");
- assertThatThrownBy(() -> jedis.smove("{user1}a-set", "{user1}a-hash",
"foo"))
- .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ @Test
+ public void smove_withWrongTypeSourceAndDest_returnsWrongTypeError() {
+ jedis.set(sourceKey, "sourceMember");
+ jedis.set(destKey, "destMember");
+
+ assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
}
@Test
- public void testSMove() {
- String source = "{user1}source";
- String dest = "{user1}dest";
- String test = "{user1}test";
- int elements = 10;
- String[] strings = generateStrings(elements, "value-");
- jedis.sadd(source, strings);
-
- long i = 1;
- for (String entry : strings) {
- long results = jedis.smove(source, dest, entry);
- assertThat(results).isEqualTo(1);
- assertThat(jedis.sismember(dest, entry)).isTrue();
-
- results = jedis.scard(source);
- assertThat(results).isEqualTo(strings.length - i);
- assertThat(jedis.scard(dest)).isEqualTo(i);
- i++;
- }
-
- assertThat(jedis.smove(test, dest, "unknown-value")).isEqualTo(0);
+ public void smove_withNonExistentSource_returnsZero_sourceKeyDoesNotExist() {
+ jedis.sadd(destKey, destMembers);
+
+ assertThat(jedis.smove(nonExistentSetKey, destKey, movedMember))
+ .isEqualTo(0);
+ assertThat(jedis.exists(nonExistentSetKey)).isFalse();
}
@Test
- public void testSMoveNegativeCases() {
- String source = "{user1}source";
- String dest = "{user1}dest";
- jedis.sadd(source, "sourceField");
- jedis.sadd(dest, "destField");
- String nonexistentField = "nonexistentField";
-
- assertThat(jedis.smove(source, dest, nonexistentField)).isEqualTo(0);
- assertThat(jedis.sismember(dest, nonexistentField)).isFalse();
- assertThat(jedis.smove(source, "{user1}nonexistentDest",
nonexistentField)).isEqualTo(0);
- assertThat(jedis.smove("{user1}nonExistentSource", dest,
nonexistentField)).isEqualTo(0);
+ public void
smove_withNonExistentMemberInSource_returnsZero_memberNotAddedToDest() {
+ String nonExistentMember = "foo";
+ jedis.sadd(sourceKey, sourceMembers);
+ jedis.sadd(destKey, destMembers);
+
+ assertThat(jedis.smove(nonExistentSetKey, destKey, nonExistentMember))
+ .isEqualTo(0);
+ assertThat(jedis.sismember(destKey, nonExistentMember)).isFalse();
+ }
+
+ @Test
+ public void
smove_withExistentSourceAndNonExistentDest_returnsOne_memberMovedFromSourceToCreatedDest()
{
+ jedis.sadd(sourceKey, sourceMembers);
+
+ String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
+ String[] destResult = new String[] {movedMember};
+
+ assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ .isEqualTo(1);
+
+
assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
+ assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destResult);
}
@Test
- public void testConcurrentSMove() {
- String source = "{user1}source";
- String dest = "{user1}dest";
- int elements = 10000;
- String[] strings = generateStrings(elements, "value-");
- jedis.sadd(source, strings);
-
- AtomicLong counter = new AtomicLong(0);
- new ConcurrentLoopingThreads(elements,
- (i) -> counter.getAndAdd(jedis.smove(source, dest, strings[i])),
- (i) -> counter.getAndAdd(jedis.smove(source, dest, strings[i]))).run();
-
- assertThat(counter.get()).isEqualTo(new Long(strings.length));
- assertThat(jedis.smembers(dest)).containsExactlyInAnyOrder(strings);
- assertThat(jedis.scard(source)).isEqualTo(0L);
+ public void
smove_withExistentSourceAndDest_returnsOne_memberMovedFromSourceToDest() {
+ jedis.sadd(sourceKey, sourceMembers);
+ jedis.sadd(destKey, destMembers);
+
+ String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
+ String[] destResult = ArrayUtils.add(destMembers, movedMember);
+
+ assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ .isEqualTo(1);
+
+
assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
+ assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destResult);
}
@Test
- public void testConcurrentSMove_withDifferentDestination() {
- String source = "{user1}source";
- String dest1 = "{user1}dest1";
- String dest2 = "{user1}dest2";
- int elements = 10000;
- String[] strings = generateStrings(elements, "value-");
- jedis.sadd(source, strings);
-
- AtomicLong counter = new AtomicLong(0);
- new ConcurrentLoopingThreads(elements,
- (i) -> counter.getAndAdd(jedis.smove(source, dest1, strings[i])),
- (i) -> counter.getAndAdd(jedis.smove(source, dest2,
strings[i]))).run();
-
- List<String> result = new ArrayList<>();
- result.addAll(jedis.smembers(dest1));
- result.addAll(jedis.smembers(dest2));
-
- assertThat(counter.get()).isEqualTo(new Long(strings.length));
- assertThat(result).containsExactlyInAnyOrder(strings);
- assertThat(jedis.scard(source)).isEqualTo(0L);
+ public void
smove_withExistentSourceAndDest_withMemberInDest_returnsOne_memberRemovedFromSource()
{
+ jedis.sadd(sourceKey, sourceMembers);
+ String[] newDestMembers = ArrayUtils.add(destMembers, movedMember);
+ jedis.sadd(destKey, newDestMembers);
+
+ String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
+
+ assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ .isEqualTo(1);
+
+
assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
+
assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(newDestMembers);
}
- private String[] generateStrings(int elements, String prefix) {
- Set<String> strings = new HashSet<>();
- for (int i = 0; i < elements; i++) {
- strings.add(prefix + i);
- }
- return strings.toArray(new String[strings.size()]);
+ @Test
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ String[] sourceMemberRemoved = ArrayUtils.remove(sourceMembers, 0);
+ String[] destMemberAdded = ArrayUtils.add(destMembers, movedMember);
+
+ jedis.sadd(sourceKey, sourceMembers);
+ jedis.sadd(destKey, destMembers);
+
+ final AtomicLong moved = new AtomicLong(0);
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.srem(sourceKey, movedMember),
+ i -> moved.set(jedis.smove(sourceKey, destKey, movedMember)))
+ .runWithAction(() -> {
+ // Check sdiffstore return size of diff
+ assertThat(moved).satisfiesAnyOf(
+ smoveResult -> assertThat(smoveResult.get()).isEqualTo(0),
+ smoveResult -> assertThat(smoveResult.get()).isEqualTo(1));
+ // Checks if values were moved or not from source key
+ assertThat(sourceKey).satisfiesAnyOf(
+ source -> assertThat(jedis.smembers(source))
+ .containsExactlyInAnyOrder(sourceMembers),
+ source -> assertThat(jedis.smembers(source))
+ .containsExactlyInAnyOrder(sourceMemberRemoved));
+ // Checks if values were moved or not to destination key
+ assertThat(destKey).satisfiesAnyOf(
+ dest -> assertThat(jedis.smembers(dest))
+ .containsExactlyInAnyOrder(destMembers),
+ dest -> assertThat(jedis.smembers(dest))
+ .containsExactlyInAnyOrder(destMemberAdded));
Review comment:
I think this can be simplified and made a bit more explicit. If moved ==
1 then the member must have moved and we should check for that. Similarly, if
moved == 0, we can check that the member was just deleted and not moved.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSMoveIntegrationTest.java
##########
@@ -53,118 +55,132 @@ public void tearDown() {
}
@Test
- public void errors_GivenWrongNumberOfArguments() {
+ public void smove_givenWrongNumberOfArguments_returnsError() {
assertExactNumberOfArgs(jedis, Protocol.Command.SMOVE, 3);
}
@Test
- public void testSmove_returnsWrongType_whenWrongSourceIsUsed() {
- jedis.set("{user1}a-string", "value");
- assertThatThrownBy(() -> jedis.smove("{user1}a-string", "{user1}some-set",
"foo"))
- .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
-
- jedis.hset("{user1}a-hash", "field", "value");
- assertThatThrownBy(() -> jedis.smove("{user1}a-hash", "{user1}some-set",
"foo"))
- .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ public void smove_withWrongTypeSource_returnsWrongTypeError() {
+ jedis.set(sourceKey, "value");
+ jedis.sadd(destKey, destMembers);
+
+ assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
}
@Test
- public void testSmove_returnsWrongType_whenWrongDestinationIsUsed() {
- jedis.sadd("{user1}a-set", "foobaz");
+ public void smove_withWrongTypeDest_returnsWrongTypeError() {
+ jedis.sadd(sourceKey, sourceMembers);
+ jedis.set(destKey, "value");
- jedis.set("{user1}a-string", "value");
- assertThatThrownBy(() -> jedis.smove("{user1}a-set", "{user1}a-string",
"foo"))
- .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
+ }
- jedis.hset("{user1}a-hash", "field", "value");
- assertThatThrownBy(() -> jedis.smove("{user1}a-set", "{user1}a-hash",
"foo"))
- .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ @Test
+ public void smove_withWrongTypeSourceAndDest_returnsWrongTypeError() {
+ jedis.set(sourceKey, "sourceMember");
+ jedis.set(destKey, "destMember");
+
+ assertThatThrownBy(() -> jedis.smove(sourceKey, destKey, movedMember))
+ .hasMessageContaining(RedisConstants.ERROR_WRONG_TYPE);
}
@Test
- public void testSMove() {
- String source = "{user1}source";
- String dest = "{user1}dest";
- String test = "{user1}test";
- int elements = 10;
- String[] strings = generateStrings(elements, "value-");
- jedis.sadd(source, strings);
-
- long i = 1;
- for (String entry : strings) {
- long results = jedis.smove(source, dest, entry);
- assertThat(results).isEqualTo(1);
- assertThat(jedis.sismember(dest, entry)).isTrue();
-
- results = jedis.scard(source);
- assertThat(results).isEqualTo(strings.length - i);
- assertThat(jedis.scard(dest)).isEqualTo(i);
- i++;
- }
-
- assertThat(jedis.smove(test, dest, "unknown-value")).isEqualTo(0);
+ public void smove_withNonExistentSource_returnsZero_sourceKeyDoesNotExist() {
+ jedis.sadd(destKey, destMembers);
+
+ assertThat(jedis.smove(nonExistentSetKey, destKey, movedMember))
+ .isEqualTo(0);
+ assertThat(jedis.exists(nonExistentSetKey)).isFalse();
}
@Test
- public void testSMoveNegativeCases() {
- String source = "{user1}source";
- String dest = "{user1}dest";
- jedis.sadd(source, "sourceField");
- jedis.sadd(dest, "destField");
- String nonexistentField = "nonexistentField";
-
- assertThat(jedis.smove(source, dest, nonexistentField)).isEqualTo(0);
- assertThat(jedis.sismember(dest, nonexistentField)).isFalse();
- assertThat(jedis.smove(source, "{user1}nonexistentDest",
nonexistentField)).isEqualTo(0);
- assertThat(jedis.smove("{user1}nonExistentSource", dest,
nonexistentField)).isEqualTo(0);
+ public void
smove_withNonExistentMemberInSource_returnsZero_memberNotAddedToDest() {
+ String nonExistentMember = "foo";
+ jedis.sadd(sourceKey, sourceMembers);
+ jedis.sadd(destKey, destMembers);
+
+ assertThat(jedis.smove(nonExistentSetKey, destKey, nonExistentMember))
+ .isEqualTo(0);
+ assertThat(jedis.sismember(destKey, nonExistentMember)).isFalse();
+ }
+
+ @Test
+ public void
smove_withExistentSourceAndNonExistentDest_returnsOne_memberMovedFromSourceToCreatedDest()
{
+ jedis.sadd(sourceKey, sourceMembers);
+
+ String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
+ String[] destResult = new String[] {movedMember};
+
+ assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ .isEqualTo(1);
+
+
assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
+ assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destResult);
}
@Test
- public void testConcurrentSMove() {
- String source = "{user1}source";
- String dest = "{user1}dest";
- int elements = 10000;
- String[] strings = generateStrings(elements, "value-");
- jedis.sadd(source, strings);
-
- AtomicLong counter = new AtomicLong(0);
- new ConcurrentLoopingThreads(elements,
- (i) -> counter.getAndAdd(jedis.smove(source, dest, strings[i])),
- (i) -> counter.getAndAdd(jedis.smove(source, dest, strings[i]))).run();
-
- assertThat(counter.get()).isEqualTo(new Long(strings.length));
- assertThat(jedis.smembers(dest)).containsExactlyInAnyOrder(strings);
- assertThat(jedis.scard(source)).isEqualTo(0L);
+ public void
smove_withExistentSourceAndDest_returnsOne_memberMovedFromSourceToDest() {
+ jedis.sadd(sourceKey, sourceMembers);
+ jedis.sadd(destKey, destMembers);
+
+ String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
+ String[] destResult = ArrayUtils.add(destMembers, movedMember);
+
+ assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ .isEqualTo(1);
+
+
assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
+ assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(destResult);
}
@Test
- public void testConcurrentSMove_withDifferentDestination() {
- String source = "{user1}source";
- String dest1 = "{user1}dest1";
- String dest2 = "{user1}dest2";
- int elements = 10000;
- String[] strings = generateStrings(elements, "value-");
- jedis.sadd(source, strings);
-
- AtomicLong counter = new AtomicLong(0);
- new ConcurrentLoopingThreads(elements,
- (i) -> counter.getAndAdd(jedis.smove(source, dest1, strings[i])),
- (i) -> counter.getAndAdd(jedis.smove(source, dest2,
strings[i]))).run();
-
- List<String> result = new ArrayList<>();
- result.addAll(jedis.smembers(dest1));
- result.addAll(jedis.smembers(dest2));
-
- assertThat(counter.get()).isEqualTo(new Long(strings.length));
- assertThat(result).containsExactlyInAnyOrder(strings);
- assertThat(jedis.scard(source)).isEqualTo(0L);
+ public void
smove_withExistentSourceAndDest_withMemberInDest_returnsOne_memberRemovedFromSource()
{
+ jedis.sadd(sourceKey, sourceMembers);
+ String[] newDestMembers = ArrayUtils.add(destMembers, movedMember);
+ jedis.sadd(destKey, newDestMembers);
+
+ String[] sourceResult = ArrayUtils.remove(sourceMembers, 0);
+
+ assertThat(jedis.smove(sourceKey, destKey, movedMember))
+ .isEqualTo(1);
+
+
assertThat(jedis.smembers(sourceKey)).containsExactlyInAnyOrder(sourceResult);
+
assertThat(jedis.smembers(destKey)).containsExactlyInAnyOrder(newDestMembers);
}
- private String[] generateStrings(int elements, String prefix) {
- Set<String> strings = new HashSet<>();
- for (int i = 0; i < elements; i++) {
- strings.add(prefix + i);
- }
- return strings.toArray(new String[strings.size()]);
+ @Test
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ String[] sourceMemberRemoved = ArrayUtils.remove(sourceMembers, 0);
+ String[] destMemberAdded = ArrayUtils.add(destMembers, movedMember);
+
+ jedis.sadd(sourceKey, sourceMembers);
+ jedis.sadd(destKey, destMembers);
+
+ final AtomicLong moved = new AtomicLong(0);
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.srem(sourceKey, movedMember),
+ i -> moved.set(jedis.smove(sourceKey, destKey, movedMember)))
+ .runWithAction(() -> {
+ // Check sdiffstore return size of diff
+ assertThat(moved).satisfiesAnyOf(
+ smoveResult -> assertThat(smoveResult.get()).isEqualTo(0),
+ smoveResult -> assertThat(smoveResult.get()).isEqualTo(1));
+ // Checks if values were moved or not from source key
+ assertThat(sourceKey).satisfiesAnyOf(
+ source -> assertThat(jedis.smembers(source))
+ .containsExactlyInAnyOrder(sourceMembers),
+ source -> assertThat(jedis.smembers(source))
+ .containsExactlyInAnyOrder(sourceMemberRemoved));
+ // Checks if values were moved or not to destination key
+ assertThat(destKey).satisfiesAnyOf(
+ dest -> assertThat(jedis.smembers(dest))
+ .containsExactlyInAnyOrder(destMembers),
+ dest -> assertThat(jedis.smembers(dest))
+ .containsExactlyInAnyOrder(destMemberAdded));
+ jedis.sadd(sourceKey, movedMember);
+ jedis.srem(destKey, movedMember);
+ });
Review comment:
In addition, it would be good to have a concurrent test that does an
`smove` in each thread using the same source but different destination keys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
