[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7321: GEODE-9993: Make SMOVE transactional

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #7321:
URL: https://github.com/apache/geode/pull/7321#discussion_r794754838



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -77,18 +77,15 @@ public RedisSet() {}
 
   public static int smove(RedisKey sourceKey, RedisKey destKey, byte[] member,
       RegionProvider regionProvider) {
-    Region<RedisKey, RedisData> region = regionProvider.getDataRegion();
-
     RedisSet source = regionProvider.getTypedRedisData(REDIS_SET, sourceKey, false);
     RedisSet destination = regionProvider.getTypedRedisData(REDIS_SET, destKey, false);
-    if (source.members.isEmpty() || !source.members.contains(member)) {
+    if (source == NULL_REDIS_SET || !source.members.contains(member)) {

Review comment:
       Consider using `source.isNull()` instead of `source == NULL_REDIS_SET`. Also I think you can get rid of this if block by checking if `source.srem` rerturns 0.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSet.java
##########
@@ -58,13 +59,13 @@ public int scard() {
   }
 
   @Override
-  public long sadd(List<byte[]> membersToAdd, Region<RedisKey, RedisData> region, RedisKey key) {
-    region.create(key, new RedisSet(membersToAdd));
+  public long sadd(List<byte[]> membersToAdd, RegionProvider regionProvider, RedisKey key) {
+    regionProvider.getLocalDataRegion().create(key, new RedisSet(membersToAdd));

Review comment:
       getLocalDataRegion is more expensive than getDataRegion and in this case we are using it to do a create. The region returned by getLocalDataRegion only allows local reads but write ops are unconstrained. Since create is a write op I think you can just change this to getDataRegion

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -77,18 +77,15 @@ public RedisSet() {}
 
   public static int smove(RedisKey sourceKey, RedisKey destKey, byte[] member,
       RegionProvider regionProvider) {
-    Region<RedisKey, RedisData> region = regionProvider.getDataRegion();
-
     RedisSet source = regionProvider.getTypedRedisData(REDIS_SET, sourceKey, false);
     RedisSet destination = regionProvider.getTypedRedisData(REDIS_SET, destKey, false);
-    if (source.members.isEmpty() || !source.members.contains(member)) {
+    if (source == NULL_REDIS_SET || !source.members.contains(member)) {
       return 0;
     }
-
-    List<byte[]> movedMember = new ArrayList<>();
-    movedMember.add(member);
-    source.srem(movedMember, region, sourceKey);
-    destination.sadd(movedMember, region, destKey);
+    List<byte[]> memberList = new ArrayList<>();
+    memberList.add(member);
+    source.srem(memberList, regionProvider, sourceKey);

Review comment:
       srem and sadd actually modifies "memberList" but they do not make clear how they modify it. So I was worried that this code might get in trouble in reusing "memberList" for the sadd call. But it turns out it is okay because srem only removes members from the list that it did not remove and sadd only removes members from the list that it did not add. I think it might be helpful to spell this out in the javadoc comment on these two methods (something like NOTE this list will be modified to only contains members removed/added).




-- 
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