[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6832: GEODE:9556: make GETSET a supported command

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

jdeppe-pivotal commented on a change in pull request #6832:
URL: https://github.com/apache/geode/pull/6832#discussion_r701566966



##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractGetSetIntegrationTest.java
##########
@@ -120,41 +114,20 @@ public void testGetSet_whenWrongType_shouldReturnError() {
   }
 
   @Test
-  public void testGetSet_shouldBeAtomic()
-      throws ExecutionException, InterruptedException {
-    jedis.set("contestedKey", "0");
-    assertThat(jedis.get("contestedKey")).isEqualTo("0");
-    CountDownLatch latch = new CountDownLatch(1);
-    ExecutorService pool = Executors.newFixedThreadPool(2);
-    Callable<Integer> callable1 = () -> doABunchOfIncrs(jedis, latch);
-    Callable<Integer> callable2 = () -> doABunchOfGetSets(jedis, latch);
-    Future<Integer> future1 = pool.submit(callable1);
-    Future<Integer> future2 = pool.submit(callable2);
-
-    latch.countDown();
-
-    GeodeAwaitility.await().untilAsserted(() -> assertThat(future2.get()).isEqualTo(future1.get()));
-    assertThat(future1.get() + future2.get()).isEqualTo(2 * ITERATION_COUNT);
-  }
-
-  private Integer doABunchOfIncrs(JedisCluster jedis, CountDownLatch latch)
-      throws InterruptedException {
-    latch.await();
-    for (int i = 0; i < ITERATION_COUNT; i++) {
-      jedis.incr("contestedKey");
-    }
-    return ITERATION_COUNT;
-  }
-
-  private Integer doABunchOfGetSets(JedisCluster jedis, CountDownLatch latch)
-      throws InterruptedException {
-    int sum = 0;
-    latch.await();
-
-    while (sum < ITERATION_COUNT) {
-      sum += Integer.parseInt(jedis.getSet("contestedKey", "0"));
-    }
-    return sum;
+  public void testGetSet_shouldBeAtomic() {
+    String contestedKey = "contestedKey";
+    jedis.set(contestedKey, "0");
+    assertThat(jedis.get(contestedKey)).isEqualTo("0");
+
+    final AtomicInteger sumHolder = new AtomicInteger(0);
+    new ConcurrentLoopingThreads(ITERATION_COUNT,
+        (ignored) -> jedis.incr(contestedKey),
+        (i) -> {
+          // continue fetching with getSet until caught up with incrementing
+          // noinspection StatementWithEmptyBody
+          while (sumHolder.getAndAdd(Integer.parseInt(jedis.getSet(contestedKey, "0"))) <= i);
+        }).run();
+    assertThat(sumHolder.get()).isEqualTo(ITERATION_COUNT);

Review comment:
       It took me a moment to figure out the while here. Consider this as an alternative perhaps?
   ```
       final AtomicInteger sumHolder = new AtomicInteger(0);
       new ConcurrentLoopingThreads(ITERATION_COUNT,
           i -> jedis.incr(contestedKey),
           i -> sumHolder.getAndAdd(Integer.parseInt(jedis.getSet(contestedKey, "0"))))
           .runInLockstep();
       sumHolder.getAndAdd(Integer.parseInt(jedis.getSet(contestedKey, "0")));
       assertThat(sumHolder.get()).isEqualTo(ITERATION_COUNT);
   ```
   The `runInLockstep` will ensure that each thread is in lockstep on the iteration so no one thread can "run ahead". Still needs a final `getSet` since either thread could complete first.




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