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

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

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



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -415,6 +413,14 @@ public long sadd(List<byte[]> membersToAdd, Region<RedisKey, RedisData> region,
     return membersAdded;
   }
 
+  void addForSmove(byte[] memberToAdd, Region<RedisKey, RedisData> region, RedisKey key) {

Review comment:
       I'm concerned that the transactional aspect of the `SMOVE` command will creep in here in a non-obvious way. I would consider adding a check at the lowest possible level to see if we're in a tx and then simply do the `region.put` there. Even so far as just adding it to `AbstractRedisData.storeChanges` although then we'd be creating a bunch of unused delta constructs during tx operations. Alternatively, do this in `sadd` and `srem`. (A tx check can be done by calling `exists()` on the `CachTransactionManager` attached to the `RegionProvider`).
   
   Ideally any redis data structure, (that extends from `AbstractRedisData`), should not contain any tx-specific code.




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