[GitHub] [geode] DonalEvans commented on a change in pull request #7160: GEODE-9826: SCARD command supported

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

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



##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSCardIntegrationTest.java
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.set;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.assertj.core.api.Assertions;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractSCardIntegrationTest implements RedisIntegrationTest {
+  private JedisCluster jedis;
+  private static final ThreePhraseGenerator generator = new ThreePhraseGenerator();
+
+  private static final String key = "{user1}setKey";
+
+  @Before
+  public void setUp() {
+    jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
+  }
+
+  @After
+  public void tearDown() {
+    flushAll();
+    jedis.close();
+  }
+
+  @Test
+  public void scardWrongNumOfArgs_returnsError() {

Review comment:
       It would be good to also have a test for the behaviour when SCARD is called with a key that is not a Redis set. We would expect a WRONGTYPE error response in that case.

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSCardIntegrationTest.java
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.set;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.assertj.core.api.Assertions;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractSCardIntegrationTest implements RedisIntegrationTest {
+  private JedisCluster jedis;
+  private static final ThreePhraseGenerator generator = new ThreePhraseGenerator();
+
+  private static final String key = "{user1}setKey";
+
+  @Before
+  public void setUp() {
+    jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
+  }
+
+  @After
+  public void tearDown() {
+    flushAll();
+    jedis.close();
+  }
+
+  @Test
+  public void scardWrongNumOfArgs_returnsError() {
+    assertExactNumberOfArgs(jedis, Protocol.Command.SCARD, 1);
+  }
+
+  @Test
+  public void scardSet_returnsSize() {
+    String[] values = new String[] {"Jeff", "Natalie", "Michelle", "Joe", "Kelley"};
+    jedis.sadd(key, values);
+    assertThat(jedis.scard(key)).isEqualTo(values.length);
+  }
+
+  @Test
+  public void scardNonExistentSet_returnsZero() {
+    assertThat(jedis.scard("{user1}nonExistentSet")).isEqualTo(0);
+  }
+
+  @Test
+  public void scardAfterSadd_returnsSize() {
+    String[] values = new String[] {"Jeff", "Natalie", "Michelle", "Joe", "Kelley"};
+
+    assertThat(jedis.scard(key)).isEqualTo(0);
+    jedis.sadd(key, values);
+
+    jedis.sadd(key, values);
+    assertThat(jedis.scard(key)).isEqualTo(values.length);
+  }
+
+  @Test
+  public void scardConcurrentSAddSCard_sameKeyPerClient()
+      throws InterruptedException, ExecutionException {
+    int elements = 1000;
+
+    String key = generator.generate('x');
+    String[] members1 = generateStrings(elements, 'y');
+    String[] members2 = generateStrings(elements, 'z');
+
+    ExecutorService pool = Executors.newFixedThreadPool(2);
+    Callable<Integer> callable1 = () -> doABunchOfSAdds(key, members1, jedis);
+    Callable<Integer> callable2 = () -> doABunchOfSAdds(key, members2, jedis);
+    Future<Integer> future1 = pool.submit(callable1);
+    Future<Integer> future2 = pool.submit(callable2);
+
+    Assertions.assertThat(future1.get()).isEqualTo(members1.length);
+    Assertions.assertThat(future2.get()).isEqualTo(members2.length);
+
+    Assertions.assertThat(jedis.scard(key)).isEqualTo(members1.length + members2.length);
+
+    pool.shutdown();
+  }
+
+  @Test
+  public void scardConcurrentSAddSCard_differentKeyPerClient()
+      throws InterruptedException, ExecutionException {
+    int elements = 1000;
+    String key1 = generator.generate('x');
+    String key2 = generator.generate('y');
+
+    String[] strings = generateStrings(elements, 'y');
+
+    ExecutorService pool = Executors.newFixedThreadPool(2);
+    Callable<Integer> callable1 = () -> doABunchOfSAdds(key1, strings, jedis);
+    Callable<Integer> callable2 = () -> doABunchOfSAdds(key2, strings, jedis);
+    Future<Integer> future1 = pool.submit(callable1);
+    Future<Integer> future2 = pool.submit(callable2);
+
+    Assertions.assertThat(future1.get()).isEqualTo(strings.length);
+    Assertions.assertThat(future2.get()).isEqualTo(strings.length);
+
+    Assertions.assertThat(jedis.scard(key1)).isEqualTo(strings.length);
+    Assertions.assertThat(jedis.scard(key2)).isEqualTo(strings.length);
+
+    pool.shutdown();
+  }
+
+  private int doABunchOfSAdds(String key, String[] strings, JedisCluster jedis) {
+    int successes = 0;
+
+    for (int i = 0; i < strings.length; i++) {
+      Long reply = jedis.sadd(key, strings[i]);
+      if (reply == 1L) {
+        successes++;
+        Thread.yield();
+      }
+    }
+    return successes;
+  }
+
+  private String[] generateStrings(int elements, char uniqueElement) {
+    Set<String> strings = new HashSet<>();
+    for (int i = 0; i < elements; i++) {
+      String elem = generator.generate(uniqueElement);

Review comment:
       A simpler way of generating unique members is to just use some pre-defined base and add the value of i to it:
   ```
     private String[] generateStrings(int elements) {
       String[] strings = new String[elements];
       for (int i = 0; i < elements; i++) {
         strings[i] = "member" + i;
       }
       return strings;
     }
   ```

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSCardIntegrationTest.java
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.set;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.assertj.core.api.Assertions;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractSCardIntegrationTest implements RedisIntegrationTest {
+  private JedisCluster jedis;
+  private static final ThreePhraseGenerator generator = new ThreePhraseGenerator();
+
+  private static final String key = "{user1}setKey";
+
+  @Before
+  public void setUp() {
+    jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
+  }
+
+  @After
+  public void tearDown() {
+    flushAll();
+    jedis.close();
+  }
+
+  @Test
+  public void scardWrongNumOfArgs_returnsError() {
+    assertExactNumberOfArgs(jedis, Protocol.Command.SCARD, 1);
+  }
+
+  @Test
+  public void scardSet_returnsSize() {
+    String[] values = new String[] {"Jeff", "Natalie", "Michelle", "Joe", "Kelley"};
+    jedis.sadd(key, values);
+    assertThat(jedis.scard(key)).isEqualTo(values.length);
+  }
+
+  @Test
+  public void scardNonExistentSet_returnsZero() {
+    assertThat(jedis.scard("{user1}nonExistentSet")).isEqualTo(0);
+  }
+
+  @Test
+  public void scardAfterSadd_returnsSize() {
+    String[] values = new String[] {"Jeff", "Natalie", "Michelle", "Joe", "Kelley"};
+
+    assertThat(jedis.scard(key)).isEqualTo(0);
+    jedis.sadd(key, values);
+
+    jedis.sadd(key, values);
+    assertThat(jedis.scard(key)).isEqualTo(values.length);
+  }
+
+  @Test
+  public void scardConcurrentSAddSCard_sameKeyPerClient()
+      throws InterruptedException, ExecutionException {
+    int elements = 1000;
+
+    String key = generator.generate('x');

Review comment:
       I think that it's not necessary to call outside of the module (`ThreePhaseGenerator` lives in the `geode-gfsh` module) to generate a unique key here, since we only use one key in this test. It should be fine to just use the `key` constant that's already defined in the class.

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSCardIntegrationTest.java
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.set;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.assertj.core.api.Assertions;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractSCardIntegrationTest implements RedisIntegrationTest {
+  private JedisCluster jedis;
+  private static final ThreePhraseGenerator generator = new ThreePhraseGenerator();
+
+  private static final String key = "{user1}setKey";
+
+  @Before
+  public void setUp() {
+    jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
+  }
+
+  @After
+  public void tearDown() {
+    flushAll();
+    jedis.close();
+  }
+
+  @Test
+  public void scardWrongNumOfArgs_returnsError() {
+    assertExactNumberOfArgs(jedis, Protocol.Command.SCARD, 1);
+  }
+
+  @Test
+  public void scardSet_returnsSize() {
+    String[] values = new String[] {"Jeff", "Natalie", "Michelle", "Joe", "Kelley"};
+    jedis.sadd(key, values);
+    assertThat(jedis.scard(key)).isEqualTo(values.length);
+  }
+
+  @Test
+  public void scardNonExistentSet_returnsZero() {
+    assertThat(jedis.scard("{user1}nonExistentSet")).isEqualTo(0);
+  }
+
+  @Test
+  public void scardAfterSadd_returnsSize() {
+    String[] values = new String[] {"Jeff", "Natalie", "Michelle", "Joe", "Kelley"};
+
+    assertThat(jedis.scard(key)).isEqualTo(0);
+    jedis.sadd(key, values);
+
+    jedis.sadd(key, values);

Review comment:
       One of these calls to `sadd()` should probably be removed. Alternatively, to make this test more distinct from the `scardSet_returnsSize()` test, we could do an initial sadd, assert that scard returns the correct size, then do an srem and/or an additional sadd and assert that scard returns the new expected value.

##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSCardIntegrationTest.java
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.set;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.assertj.core.api.Assertions;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractSCardIntegrationTest implements RedisIntegrationTest {
+  private JedisCluster jedis;
+  private static final ThreePhraseGenerator generator = new ThreePhraseGenerator();
+
+  private static final String key = "{user1}setKey";
+
+  @Before
+  public void setUp() {
+    jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
+  }
+
+  @After
+  public void tearDown() {
+    flushAll();
+    jedis.close();
+  }
+
+  @Test
+  public void scardWrongNumOfArgs_returnsError() {
+    assertExactNumberOfArgs(jedis, Protocol.Command.SCARD, 1);
+  }
+
+  @Test
+  public void scardSet_returnsSize() {
+    String[] values = new String[] {"Jeff", "Natalie", "Michelle", "Joe", "Kelley"};
+    jedis.sadd(key, values);
+    assertThat(jedis.scard(key)).isEqualTo(values.length);
+  }
+
+  @Test
+  public void scardNonExistentSet_returnsZero() {
+    assertThat(jedis.scard("{user1}nonExistentSet")).isEqualTo(0);
+  }
+
+  @Test
+  public void scardAfterSadd_returnsSize() {
+    String[] values = new String[] {"Jeff", "Natalie", "Michelle", "Joe", "Kelley"};
+
+    assertThat(jedis.scard(key)).isEqualTo(0);
+    jedis.sadd(key, values);
+
+    jedis.sadd(key, values);
+    assertThat(jedis.scard(key)).isEqualTo(values.length);
+  }
+
+  @Test
+  public void scardConcurrentSAddSCard_sameKeyPerClient()

Review comment:
       This test, and the other concurrent one in this class aren't actually testing the concurrency behaviour of scard, since they're doing concurrent sadd calls, then once the sadds are done, calling scard. 
   
   In order to test concurrency for this command, we should be following a similar pattern to the one in the SDIFF PR, using `ConcurrentLoopingThreads` with:
    - One thread removing all members from the set (or maybe all but one, so that the set doesn't get removed from the region)
    - One thread calling scard and storing the result in an AtomicInteger
    - A `runWithAction()` that asserts that the stored returned value of scard is either the original size (scard was executed before srem) or 0 (srem was executed before scard)
    - Finally in the `runWithAction()`, add back in all the original members of the set so that the next iteration has the same starting conditions.




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