ezoerner commented on a change in pull request #6832:
URL: https://github.com/apache/geode/pull/6832#discussion_r701574669
##########
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:
The while loop is there pretty much to make sure there is the final
getSet, prevents the two threads from getting completely out of sync, and also
to fit in better with the iterations of the ConcurrentLoopingThreads. Keeping
them in lockstep might be ok too, though it seems hard to say whether it would
make it more or less likely to try and break the atomicity--i.e. for the
incrementing thread to theoretically try to intervene between the get and set.
I was hoping the comment above would make it a little clearer why the while
loop is there.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
