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