You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/01/05 22:08:24 UTC

[GitHub] [geode] Kris-10-0 opened a new pull request #7240: SUNION Command Support

Kris-10-0 opened a new pull request #7240:
URL: https://github.com/apache/geode/pull/7240


   <!-- Thank you for submitting a contribution to Apache Geode. -->
   
   <!-- In order to streamline the review of the contribution we ask you
   to ensure the following steps have been taken: 
   -->
   
   ### For all changes:
   - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
   
   - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
   
   - [x] Is your initial contribution a single, squashed commit?
   
   - [x] Does `gradlew build` run cleanly?
   
   - [x] Have you written or updated unit tests to verify your changes?
   
   - [NA] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   <!-- Note:
   Please ensure that once the PR is submitted, check Concourse for build issues and
   submit an update to your PR as soon as possible. If you need help, please send an
   email to dev@geode.apache.org.
   -->
   


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



[GitHub] [geode] Kris-10-0 commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r780606838



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }

Review comment:
       No I don't think so. I'll keep the  one testing three sets.

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }

Review comment:
       No I don't think so. I'll remove the three sets.




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



[GitHub] [geode] Kris-10-0 commented on pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on pull request #7240:
URL: https://github.com/apache/geode/pull/7240#issuecomment-1009565972


   Forced push to rebase


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



[GitHub] [geode] Kris-10-0 commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r779233010



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -32,16 +38,17 @@
 
 import org.apache.geode.redis.ConcurrentLoopingThreads;
 import org.apache.geode.redis.RedisIntegrationTest;
-import org.apache.geode.test.awaitility.GeodeAwaitility;
 
 public abstract class AbstractSUnionIntegrationTest implements RedisIntegrationTest {
   private JedisCluster jedis;
-  private static final int REDIS_CLIENT_TIMEOUT =
-      Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+  private static final String setKey1 = "{user1}setKey1";
+  private static final String[] setMembers1 = {"one", "two", "three", "four", "five"};
+  private static final String nonExistentSetKey = "{user1}nonExistentSet";
+  private static final String setKey2 = "{user1}setKey2";

Review comment:
       Yes! A lot of my previous tests use {user1} as a tag. Should I go ahead and change everything with this commit since its quick, create a new ticket, or just fix this and from now on use a different tag?




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



[GitHub] [geode] DonalEvans commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r779223113



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -74,12 +74,26 @@ public RedisSet(int expectedSize) {
    */
   public RedisSet() {}
 
+  public static Set<byte[]> sunion(RegionProvider regionProvider, List<RedisKey> keys) {
+    MemberSet result = calculateUnion(regionProvider, keys, true);
+    return getSetOpResult(result);
+  }
+
+  private static MemberSet calculateUnion(RegionProvider regionProvider, List<RedisKey> keys,
+      boolean updateStats) {
+    MemberSet union = new MemberSet();
+    for (RedisKey key : keys) {
+      RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET, key, updateStats);
+      if (curSet.scard() != 0) {
+        union.addAll(curSet.members);
+      }

Review comment:
       I think it should be fine to not check the cardinality of the current set here, since if it's empty, calling `addAll()` with it won't actually result in any work being done.

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsFromDifferentSlots_returnsCrossSlotError() {
+    String setKeyDifferentSlot = "{user2}setKey2";
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKeyDifferentSlot, secondSetMembers);
+
+    assertThatThrownBy(() -> jedis.sunion(setKey1, setKeyDifferentSlot))
+        .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+  }
+
+
+  @Test
+  public void sunionWithDifferentKeyType_returnsWrongTypeError() {
+    jedis.set("ding", "dong");
+    assertThatThrownBy(() -> jedis.sunion("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
+  }
+
+  @Test
+  public void ensureSetConsistency_whenRunningConcurrently() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] unionMembers = {"one", "two", "three", "four", "five", "linux", "peach"};
+    Set<String> unionList = new HashSet<>(Arrays.asList(unionMembers));
+    Set<String> firstSetMembersList = new HashSet<>(Arrays.asList(setMembers1));
+
+
+    final AtomicReference<Set<String>> sunionResultReference = new AtomicReference<>();
+    new ConcurrentLoopingThreads(1000,
+        i -> jedis.srem(setKey2, secondSetMembers),
+        i -> sunionResultReference.set(jedis.sunion(setKey1, setKey2)))
+            .runWithAction(() -> {
+              assertThat(sunionResultReference).satisfiesAnyOf(
+                  sunionResult -> assertThat(sunionResult.get())
+                      .containsExactlyInAnyOrderElementsOf(firstSetMembersList),
+                  sunionResult -> assertThat(sunionResult.get())
+                      .containsExactlyInAnyOrderElementsOf(unionList));

Review comment:
       These assertions can be simplified to:
   ```
                 assertThat(sunionResultReference).satisfiesAnyOf(
                     sunionResult -> assertThat(sunionResult.get())
                         .containsExactlyInAnyOrder(setMembers1),
                     sunionResult -> assertThat(sunionResult.get())
                         .containsExactlyInAnyOrder(unionMembers));
   ```
   which means we no longer need the `unionList` and `firstSetMembersList` variables.

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -32,16 +38,17 @@
 
 import org.apache.geode.redis.ConcurrentLoopingThreads;
 import org.apache.geode.redis.RedisIntegrationTest;
-import org.apache.geode.test.awaitility.GeodeAwaitility;
 
 public abstract class AbstractSUnionIntegrationTest implements RedisIntegrationTest {
   private JedisCluster jedis;
-  private static final int REDIS_CLIENT_TIMEOUT =
-      Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+  private static final String setKey1 = "{user1}setKey1";
+  private static final String[] setMembers1 = {"one", "two", "three", "four", "five"};
+  private static final String nonExistentSetKey = "{user1}nonExistentSet";
+  private static final String setKey2 = "{user1}setKey2";

Review comment:
       Just to avoid potential confusion, could we use a different hash tag on the keys here? The tag has nothing to do with users, just to which slot keys map to, so there's a potential for confusion with this naming. Maybe just "{tag}setKey1" etc. to keep it simple?

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }

Review comment:
       This test doesn't seem to be testing anything not already covered in the tests with two sets. Are both necessary?

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -74,12 +74,26 @@ public RedisSet(int expectedSize) {
    */
   public RedisSet() {}
 
+  public static Set<byte[]> sunion(RegionProvider regionProvider, List<RedisKey> keys) {
+    MemberSet result = calculateUnion(regionProvider, keys, true);
+    return getSetOpResult(result);

Review comment:
       I think it should be fine to just use 
   ```
   return calculateUnion(regionProvider, keys, true);
   ```
   here, since that never returns null.
   
   Ideally, none of the calculate* methods should return null, but rather an empty MemberSet which can then safely be returned from the calling method, removing the need for the `getSetOpResult()` method.

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsFromDifferentSlots_returnsCrossSlotError() {
+    String setKeyDifferentSlot = "{user2}setKey2";
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKeyDifferentSlot, secondSetMembers);
+
+    assertThatThrownBy(() -> jedis.sunion(setKey1, setKeyDifferentSlot))
+        .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+  }
+
+
+  @Test
+  public void sunionWithDifferentKeyType_returnsWrongTypeError() {

Review comment:
       Could we also get a test where the first key passed to SUNION is a set, but the second key is some other type please?




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



[GitHub] [geode] DonalEvans commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r783271880



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,95 +62,185 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{tag1}set1", firstSet);
-    jedis.sadd("{tag1}set2", secondSet);
-    jedis.sadd("{tag1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{tag1}set1", "{tag1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{tag1}nonexistent", "{tag1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithSetsFromDifferentSlots_returnsCrossSlotError() {
+    String setKeyDifferentSlot = "{user2}setKey2";
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKeyDifferentSlot, secondSetMembers);
+
+    assertThatThrownBy(() -> jedis.sunion(setKey1, setKeyDifferentSlot))
+        .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+  }
+
+
+  @Test
+  public void sunion_withDifferentKeyTypeAndTwoSetKeys_returnsWrongTypeError() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String diffKey = "{tag1}diffKey";
+    jedis.set(diffKey, "dong");
+    assertThatThrownBy(() -> jedis.sunion(diffKey, setKey1, setKey2))
+        .hasMessageContaining(ERROR_WRONG_TYPE);
+  }
 
-    jedis.sadd("{tag1}newEmpty", "born2die");
-    jedis.srem("{tag1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{tag1}set2", "{tag1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunion_withTwoSetKeysAndDifferentKeyType_returnsWrongTypeError() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String diffKey = "{tag1}diffKey";
+    jedis.set(diffKey, "dong");
+    assertThatThrownBy(() -> jedis.sunion(setKey1, setKey2, diffKey))
+        .hasMessageContaining(ERROR_WRONG_TYPE);
+  }
+
+  @Test
+  public void ensureSetConsistency_whenRunningConcurrently() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] unionMembers = {"one", "two", "three", "four", "five", "linux", "peach"};
+
+    final AtomicReference<Set<String>> sunionResultReference = new AtomicReference<>();
+    new ConcurrentLoopingThreads(1000,
+        i -> jedis.srem(setKey2, secondSetMembers),
+        i -> sunionResultReference.set(jedis.sunion(setKey1, setKey2)))
+            .runWithAction(() -> {
+              assertThat(sunionResultReference).satisfiesAnyOf(
+                  sunionResult -> assertThat(sunionResult.get())
+                      .containsExactlyInAnyOrder(setMembers1),
+                  sunionResult -> assertThat(sunionResult.get())
+                      .containsExactlyInAnyOrder(unionMembers));
+              jedis.sadd(setKey2, unionMembers);
+            });
   }
 
   @Test
   public void testSUnionStore() {
     String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
     String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
     String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{tag1}set1", firstSet);
-    jedis.sadd("{tag1}set2", secondSet);
-    jedis.sadd("{tag1}set3", thirdSet);
+    jedis.sadd("{user1}set1", firstSet);
+    jedis.sadd("{user1}set2", secondSet);
+    jedis.sadd("{user1}set3", thirdSet);

Review comment:
       These (and the other similar keys in this class) should be replaced with the `setKey1`, `setKey2` constants, and the tag on the third key (and other non-constant key names) should be replaced with "{tag1}" to be consistent with the changes from GEODE-9932.




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



[GitHub] [geode] DonalEvans commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r782635588



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsFromDifferentSlots_returnsCrossSlotError() {
+    String setKeyDifferentSlot = "{user2}setKey2";
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKeyDifferentSlot, secondSetMembers);
+
+    assertThatThrownBy(() -> jedis.sunion(setKey1, setKeyDifferentSlot))
+        .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+  }
+
+
+  @Test
+  public void sunionWithDifferentKeyType_returnsWrongTypeError() {
+    jedis.set("ding", "dong");
+    assertThatThrownBy(() -> jedis.sunion("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
+  }
+
+  @Test
+  public void ensureSetConsistency_whenRunningConcurrently() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] unionMembers = {"one", "two", "three", "four", "five", "linux", "peach"};
+    Set<String> unionList = new HashSet<>(Arrays.asList(unionMembers));
+    Set<String> firstSetMembersList = new HashSet<>(Arrays.asList(setMembers1));
+
+
+    final AtomicReference<Set<String>> sunionResultReference = new AtomicReference<>();
+    new ConcurrentLoopingThreads(1000,
+        i -> jedis.srem(setKey2, secondSetMembers),
+        i -> sunionResultReference.set(jedis.sunion(setKey1, setKey2)))
+            .runWithAction(() -> {
+              assertThat(sunionResultReference).satisfiesAnyOf(
+                  sunionResult -> assertThat(sunionResult.get())
+                      .containsExactlyInAnyOrderElementsOf(firstSetMembersList),
+                  sunionResult -> assertThat(sunionResult.get())
+                      .containsExactlyInAnyOrderElementsOf(unionList));

Review comment:
       When I use the code pasted above, the test works fine. Note that it's using `containsExactlyInAnyOrder()` rather than `containsExactlyInAnyOrderElementsOf()`.




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



[GitHub] [geode] Kris-10-0 commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r780606838



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }

Review comment:
       No I don't think so. I'll remove the three sets.




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



[GitHub] [geode] Kris-10-0 commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r780606838



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }

Review comment:
       No I don't think so. I'll keep the  one testing three sets.




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



[GitHub] [geode] Kris-10-0 commented on pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on pull request #7240:
URL: https://github.com/apache/geode/pull/7240#issuecomment-1006117526


   Forced pushed to rename commit


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



[GitHub] [geode] Kris-10-0 commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r782624382



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -56,31 +63,122 @@ public void sunionErrors_givenTooFewArguments() {
   }
 
   @Test
-  public void sunionstroreErrors_givenTooFewArgumentst() {
-    assertAtLeastNArgs(jedis, Protocol.Command.SUNIONSTORE, 2);
+  public void sunion_returnsAllValuesInSet_setNotModified() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1)).containsExactlyInAnyOrder(setMembers1);
+    assertThat(jedis.smembers(setKey1)).containsExactlyInAnyOrder(setMembers1);
   }
 
   @Test
-  public void testSUnion() {
-    String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
-    String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
-    String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
-    jedis.sadd("{user1}set1", firstSet);
-    jedis.sadd("{user1}set2", secondSet);
-    jedis.sadd("{user1}set3", thirdSet);
+  public void sunionWithNonExistentSet_returnsEmptySet_nonExistentKeyDoesNotExist() {
+    assertThat(jedis.sunion(nonExistentSetKey)).isEmpty();
+    assertThat(jedis.exists(nonExistentSetKey)).isFalse();
+  }
 
-    Set<String> resultSet = jedis.sunion("{user1}set1", "{user1}set2");
-    String[] expected =
-        new String[] {"pear", "apple", "plum", "orange", "peach", "microsoft", "linux"};
-    assertThat(resultSet).containsExactlyInAnyOrder(expected);
+  @Test
+  public void sunionWithNonExistentSetAndSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(nonExistentSetKey, setKey1)).containsExactlyInAnyOrder(setMembers1);
+  }
 
-    Set<String> otherResultSet = jedis.sunion("{user1}nonexistent", "{user1}set1");
-    assertThat(otherResultSet).containsExactlyInAnyOrder(firstSet);
+  @Test
+  public void sunionWithSetAndNonExistentSet_returnsAllValuesInSet() {
+    jedis.sadd(setKey1, setMembers1);
+    assertThat(jedis.sunion(setKey1, nonExistentSetKey))
+        .containsExactlyInAnyOrder(setMembers1);
+  }
 
-    jedis.sadd("{user1}newEmpty", "born2die");
-    jedis.srem("{user1}newEmpty", "born2die");
-    Set<String> yetAnotherResultSet = jedis.sunion("{user1}set2", "{user1}newEmpty");
-    assertThat(yetAnotherResultSet).containsExactlyInAnyOrder(secondSet);
+  @Test
+  public void sunionWithMultipleNonExistentSets_returnsEmptySet() {
+    assertThat(jedis.sunion("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+  }
+
+  @Test
+  public void sunionWithNonOverlappingSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"apple", "microsoft", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "apple", "microsoft", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithSomeSharedValues_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] result = {"one", "two", "three", "four", "five", "linux", "peach"};
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsWithAllSharedValues_returnsUnionOfSets() {
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, setMembers1);
+
+    assertThat(jedis.sunion(setKey1, setKey2)).containsExactlyInAnyOrder(setMembers1);
+  }
+
+  @Test
+  public void sunionWithMultipleSets_returnsUnionOfSets() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    String thirdSetKey = "{user1}setKey3";
+    String[] thirdSetMembers = new String[] {"fire", "dance", "floor", "five"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+    jedis.sadd(thirdSetKey, thirdSetMembers);
+
+
+    String[] result =
+        {"one", "two", "three", "four", "five", "linux", "peach", "fire", "dance", "floor"};
+    assertThat(jedis.sunion(setKey1, setKey2, thirdSetKey))
+        .containsExactlyInAnyOrder(result);
+  }
+
+  @Test
+  public void sunionWithSetsFromDifferentSlots_returnsCrossSlotError() {
+    String setKeyDifferentSlot = "{user2}setKey2";
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKeyDifferentSlot, secondSetMembers);
+
+    assertThatThrownBy(() -> jedis.sunion(setKey1, setKeyDifferentSlot))
+        .hasMessageContaining(ERROR_DIFFERENT_SLOTS);
+  }
+
+
+  @Test
+  public void sunionWithDifferentKeyType_returnsWrongTypeError() {
+    jedis.set("ding", "dong");
+    assertThatThrownBy(() -> jedis.sunion("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
+  }
+
+  @Test
+  public void ensureSetConsistency_whenRunningConcurrently() {
+    String[] secondSetMembers = new String[] {"one", "two", "linux", "peach"};
+    jedis.sadd(setKey1, setMembers1);
+    jedis.sadd(setKey2, secondSetMembers);
+
+    String[] unionMembers = {"one", "two", "three", "four", "five", "linux", "peach"};
+    Set<String> unionList = new HashSet<>(Arrays.asList(unionMembers));
+    Set<String> firstSetMembersList = new HashSet<>(Arrays.asList(setMembers1));
+
+
+    final AtomicReference<Set<String>> sunionResultReference = new AtomicReference<>();
+    new ConcurrentLoopingThreads(1000,
+        i -> jedis.srem(setKey2, secondSetMembers),
+        i -> sunionResultReference.set(jedis.sunion(setKey1, setKey2)))
+            .runWithAction(() -> {
+              assertThat(sunionResultReference).satisfiesAnyOf(
+                  sunionResult -> assertThat(sunionResult.get())
+                      .containsExactlyInAnyOrderElementsOf(firstSetMembersList),
+                  sunionResult -> assertThat(sunionResult.get())
+                      .containsExactlyInAnyOrderElementsOf(unionList));

Review comment:
       I couldn't put those directly because it was it was saying I needed an iterator. 




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



[GitHub] [geode] DonalEvans commented on a change in pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7240:
URL: https://github.com/apache/geode/pull/7240#discussion_r779692869



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSUnionIntegrationTest.java
##########
@@ -32,16 +38,17 @@
 
 import org.apache.geode.redis.ConcurrentLoopingThreads;
 import org.apache.geode.redis.RedisIntegrationTest;
-import org.apache.geode.test.awaitility.GeodeAwaitility;
 
 public abstract class AbstractSUnionIntegrationTest implements RedisIntegrationTest {
   private JedisCluster jedis;
-  private static final int REDIS_CLIENT_TIMEOUT =
-      Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+  private static final String setKey1 = "{user1}setKey1";
+  private static final String[] setMembers1 = {"one", "two", "three", "four", "five"};
+  private static final String nonExistentSetKey = "{user1}nonExistentSet";
+  private static final String setKey2 = "{user1}setKey2";

Review comment:
       I think just changing this file for now would be fine. I'll file a ticket for renaming the rest of the hash tags today.




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



[GitHub] [geode] Kris-10-0 commented on pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on pull request #7240:
URL: https://github.com/apache/geode/pull/7240#issuecomment-1007801822


   Forced push to rebase


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



[GitHub] [geode] Kris-10-0 commented on pull request #7240: feature/GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on pull request #7240:
URL: https://github.com/apache/geode/pull/7240#issuecomment-1007801822


   Forced push to rebase


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



[GitHub] [geode] nonbinaryprogrammer merged pull request #7240: GEODE-9836: SUNION Command Support

Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer merged pull request #7240:
URL: https://github.com/apache/geode/pull/7240


   


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