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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
