[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7157: GEODE-9827: SDIFF command supported

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

dschneider-pivotal commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760502200



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
##########
@@ -41,12 +43,36 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
 
     List<RedisKey> commandElements = command.getProcessedCommandKeys();
     List<RedisKey> setKeys = commandElements.subList(setsStartIndex, commandElements.size());
-    if (isStorage()) {
-      RedisKey destination = command.getKey();
-      int storeCount = doStoreSetOp(command.getCommandType(), context, destination, setKeys);
-      return RedisResponse.integer(storeCount);
+    RegionProvider regionProvider = context.getRegionProvider();
+    try {
+      for (RedisKey k : setKeys) {
+        regionProvider.ensureKeyIsLocal(k);
+      }
+    } catch (RedisDataMovedException ex) {
+      return RedisResponse.error(ex.getMessage());
+    }
+
+    /*
+     * SDIFFSTORE, SINTER, SINTERSTORE, SUNION, SUNIONSTORE currently use the else part of the code
+     * for their
+     * implementation.
+     * TODO: Once the above commands have been implemented remove the if else and
+     * refactor so it implements doSetOp
+     */
+
+    if (command.isOfType(RedisCommandType.SDIFF)) {
+      Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), new ArrayList<>(setKeys),

Review comment:
       Could you just pass in "setKeys" instead of "new ArrayList<>(setKeys)"? setKeys is a sublist but I think it allows changes to the sublist which seems okay in this context.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +67,35 @@ public RedisSet(Collection<byte[]> members) {
    */
   public RedisSet() {}
 
+  public RedisSet(int size) {
+    this.members = new RedisSet.MemberSet(size);
+  }
+
+  public Set<byte[]> sdiff(RegionProvider regionProvider, List<RedisKey> keys) {
+    return calculateDiff(regionProvider, keys);
+  }
+
+  private Set<byte[]> calculateDiff(RegionProvider regionProvider, List<RedisKey> keys) {
+    RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET, keys.get(0), false);
+    if (firstSet == NULL_REDIS_SET) {

Review comment:
       better would be "if (firstSet.scard() == 0)". It is easier to understand since it deals with the firstSet being empty instead of the special NULL_REDIS_SET.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +67,35 @@ public RedisSet(Collection<byte[]> members) {
    */
   public RedisSet() {}
 
+  public RedisSet(int size) {
+    this.members = new RedisSet.MemberSet(size);
+  }
+
+  public Set<byte[]> sdiff(RegionProvider regionProvider, List<RedisKey> keys) {
+    return calculateDiff(regionProvider, keys);
+  }
+
+  private Set<byte[]> calculateDiff(RegionProvider regionProvider, List<RedisKey> keys) {
+    RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET, keys.get(0), false);
+    if (firstSet == NULL_REDIS_SET) {
+      return Collections.emptySet();
+    }
+    members.addAll(firstSet.members);
+
+    for (int i = 1; i < keys.size(); i++) {
+      RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET, keys.get(i), false);
+      if (curSet == NULL_REDIS_SET) {

Review comment:
       better would be "if (curSet.scard() == 0)".

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +67,35 @@ public RedisSet(Collection<byte[]> members) {
    */
   public RedisSet() {}
 
+  public RedisSet(int size) {
+    this.members = new RedisSet.MemberSet(size);
+  }
+
+  public Set<byte[]> sdiff(RegionProvider regionProvider, List<RedisKey> keys) {

Review comment:
       I don't like sdiff being an instance method on RedisSet. It seems like the implementation counts on it always starting with an empty RedisSet. And that is how we use it. So it seems like the only RedisSet instances we care about are the existing ones we will read. So I think it is a bit confusing to create an empty RedisSet instance and then throw it away after we get the members out of it. I think it would be better for sdiff and calculateDIff to be "static" and the "members" in calculateDiff to be a local variable that is created like so: "Set<byte[]> members = new MemberSet(firstSet.members);". This will also give you a better initial size that will do fewer rehashes.
   Also you should probably rename "members" to something like "diff".
   Instead of static methods on RedisSet these could be on SetOpExecutor but since they read the internals of the RedisSet instances I think it is better on RedisSet.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +67,35 @@ public RedisSet(Collection<byte[]> members) {
    */
   public RedisSet() {}
 
+  public RedisSet(int size) {
+    this.members = new RedisSet.MemberSet(size);

Review comment:
       Since this line is in RedisSet can you change it to "new MemberSet(size)"?

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -125,6 +180,19 @@ public void testSDiffStore() {
     assertThat(copyResultSet.toArray()).containsExactlyInAnyOrder((Object[]) secondSet);
   }
 
+  // One array

Review comment:
       it is not clear what the purpose of this comment is. Do you want to remove it? If not can you add an explanation.




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