DonalEvans commented on a change in pull request #7321:
URL: https://github.com/apache/geode/pull/7321#discussion_r794915135
##########
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:
What if I refactored the sadd and srem methods to not modify the list?
That sort of side effect feels a bit risky to me to begin with, and it's not
necessary if we just iterate the list and add to the delta as we go rather than
creating the delta after doing all the adds/removes. The only real performance
difference is that if we do an SADD or SREM that ends up not adding or removing
anything, now we would be creating a single `DeltaInfo` that we weren't
previously, which isn't too expensive.
--
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
