You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/01/04 21:12:44 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #7227: feature/GEODE-9832: SMOVE Command Support
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org