DonalEvans commented on a change in pull request #7227:
URL: https://github.com/apache/geode/pull/7227#discussion_r778377500
##########
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);
Review comment:
It's not enough to just check that both keys are local. Rather, we need
to check that both keys map to the same slot, and that the bucket associated
with that slot is local.
##########
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);
Review comment:
Rather than creating a `RedisKey` for every element in `command`, then
only using a subset of them, it would be better to explicitly get the source
key and destination key byte arrays and create a `RedisKey` for each, similar
to how the rename command is implemented in `AbstractRenameExecutor`. This
results in slightly more verbose code, as you can't iterate the list of keys
and would have to pass each individually into the `smove()` method, but since
we know there are only two, this isn't a huge problem.
Doing it this way would also mean that you don't have to create a second
ArrayList for use by the `lockedExecute()` method, since the `smove()` method
would no longer be working on the same list as the one passed to
`lockedExecute()` and so modification of that list is no longer an issue, so
this would result in slightly smaller memory overhead.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -69,6 +69,30 @@ public RedisSet(int expectedSize) {
*/
public RedisSet() {}
+ public static int smove(RegionProvider regionProvider, List<RedisKey> keys,
byte[] member) {
+ Region<RedisKey, RedisData> region = regionProvider.getDataRegion();
+ RedisKey sourceKey = keys.get(0);
+ RedisKey destKey = keys.get(1);
+
+ RedisSet source = regionProvider.getTypedRedisData(REDIS_SET, sourceKey,
false);
+ if (source.scard() == 0 || !source.members.contains(member)) {
Review comment:
This would be more consistent as:
```
if (source.members.isEmpty() || !source.members.contains(member)) {
```
rather than mixing use of internal and public access to the `members`
collection.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSMoveIntegrationTest.java
##########
@@ -34,16 +33,19 @@
import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
import org.apache.geode.redis.internal.RedisConstants;
-import org.apache.geode.test.awaitility.GeodeAwaitility;
public abstract class AbstractSMoveIntegrationTest implements
RedisIntegrationTest {
Review comment:
It would be good to also have a test showing the behaviour when the
source and destination key do not map to the same slot.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -69,6 +69,30 @@ public RedisSet(int expectedSize) {
*/
public RedisSet() {}
+ public static int smove(RegionProvider regionProvider, List<RedisKey> keys,
byte[] member) {
+ Region<RedisKey, RedisData> region = regionProvider.getDataRegion();
+ RedisKey sourceKey = keys.get(0);
+ RedisKey destKey = keys.get(1);
+
+ RedisSet source = regionProvider.getTypedRedisData(REDIS_SET, sourceKey,
false);
+ if (source.scard() == 0 || !source.members.contains(member)) {
+ return 0;
+ }
+
+ RedisSet destination = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(1), false);
+
+ List<byte[]> movedMember = new ArrayList<>();
+ movedMember.add(member);
+ source.srem(movedMember, region, sourceKey);
+ if (destination.scard() == 0) {
+ region.put(destKey, new RedisSet(movedMember));
+ } else {
+ destination.sadd(movedMember, region, destKey);
+ }
Review comment:
Because of the implementation of `sadd()` in `NullRedisSet`, this
if/else is not necessary. Just calling `destination.sadd(movedMember, region,
destKey);` will result in the correct behaviour in both cases.
--
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]
