[GitHub] [geode] DonalEvans commented on a change in pull request #6544: GEODE-9336: Implement ZADD option CH

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

DonalEvans commented on a change in pull request #6544:
URL: https://github.com/apache/geode/pull/6544#discussion_r644323618



##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZAddIntegrationTest.java
##########
@@ -160,6 +160,40 @@ public void zaddCountsOnlyNewMembers_givenMultipleCopiesOfTheSameMember_toAnExis
     assertThat(jedis.zscore(SORTED_SET_KEY, "member")).isEqualTo(3.0);
   }
 
+  @Test
+  public void zaddCountsNewMemberAndExistingMemberChanges_whenCHSpecified() {

Review comment:
       Would it be possible to break this test up into two separate cases, one which tests that the CH option works for updated members, and one which tests that it works with new members? At unit and integration test level, it's generally better to try and test one thing and one thing only per test. I suppose there could also be a third case testing that specifying CH does not return anything when no members are updated or added, but maybe that behaviour could be included in an existing test case for zadd with no new members specified.

##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZAddIntegrationTest.java
##########
@@ -160,6 +160,40 @@ public void zaddCountsOnlyNewMembers_givenMultipleCopiesOfTheSameMember_toAnExis
     assertThat(jedis.zscore(SORTED_SET_KEY, "member")).isEqualTo(3.0);
   }
 
+  @Test
+  public void zaddCountsNewMemberAndExistingMemberChanges_whenCHSpecified() {
+    Map<String, Double> initMap = makeMemberScoreMap(2 * INITIAL_MEMBER_COUNT, 0);
+    Long addCount = (Long) jedis.zadd(SORTED_SET_KEY, initMap);

Review comment:
       The casts to `Long` are not necessary in this test.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/BaseSetOptions.java
##########
@@ -47,6 +47,10 @@ public boolean isXX() {
     return exists.equals(Exists.XX);
   }
 
+  public boolean isCH() {
+    return exists.equals(Exists.CH);
+  }
+

Review comment:
       I believe it's possible to set CH and one of NX or XX, so this won't work correctly, as it makes all three options mutually exclusive. I think a boolean flag for `isCH` would be better.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -189,18 +189,20 @@ long zadd(Region<RedisKey, RedisData> region, RedisKey key, List<byte[]> members
     Iterator<byte[]> iterator = membersToAdd.iterator();
 
     int initialSize = getSortedSetSize();
-
+    int changesCount = 0;
     while (iterator.hasNext()) {
       byte[] score = iterator.next();
       validateScoreIsDouble(score);
       byte[] member = iterator.next();
-
       if (options.isNX() && members.containsKey(member)) {
         continue;
       }
       if (options.isXX() && !members.containsKey(member)) {
         continue;
       }
+      if (options.isCH() && members.containsKey(member)) {
+        changesCount = getChangesCount(changesCount, score, member);
+      }

Review comment:
       Since `memberAdd` returns the old value from the internal map, it might be more efficient to simply compare that to the new value and if it's different, increment `changesCount`. That way we're not doing a bunch of the same work that the map does as part of a `put()` call just to determine if the value was updated.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org