From d8f283bfec54f5d61ec8b76f6ffb282c03b6e4c0 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 19 Jun 2024 15:33:52 -0700 Subject: [PATCH 1/4] Reworked testConcurrentRemoval Signed-off-by: Peter Alfonsi --- .../stats/DefaultCacheStatsHolderTests.java | 78 ++++++++++++------- 1 file changed, 49 insertions(+), 29 deletions(-) diff --git a/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java b/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java index c6e8252ddf806..4ae56f47996b9 100644 --- a/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java +++ b/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java @@ -20,6 +20,7 @@ import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Phaser; public class DefaultCacheStatsHolderTests extends OpenSearchTestCase { private final String storeName = "dummy_store"; @@ -127,49 +128,68 @@ public void testCount() throws Exception { } public void testConcurrentRemoval() throws Exception { - List dimensionNames = List.of("dim1", "dim2"); + List dimensionNames = List.of("A", "B"); DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames, storeName); // Create stats for the following dimension sets - List> populatedStats = List.of(List.of("A1", "B1"), List.of("A2", "B2"), List.of("A2", "B3")); + List> populatedStats = new ArrayList<>(); + int numAValues = 10; + int numBValues = 2; + for (int indexA = 0; indexA < numAValues; indexA++) { + for (int indexB = 0; indexB < numBValues; indexB++) { + populatedStats.add(List.of("A" + indexA, "B" + indexB)); + } + } for (List dims : populatedStats) { cacheStatsHolder.incrementHits(dims); } - // Remove (A2, B2) and (A1, B1), before re-adding (A2, B2). At the end we should have stats for (A2, B2) but not (A1, B1). + // Remove a subset of the dimensions concurrently. + // Remove both (A0, B0), and (A0, B1), so we expect the intermediate node for A0 to be null afterwards. + // For all the others, remove only the B0 value. Then we expect the intermediate nodes for A1 through A9 to be present + // and reflect only the stats for their B1 child. - Thread[] threads = new Thread[3]; - CountDownLatch countDownLatch = new CountDownLatch(3); - threads[0] = new Thread(() -> { - cacheStatsHolder.removeDimensions(List.of("A2", "B2")); - countDownLatch.countDown(); - }); - threads[1] = new Thread(() -> { - cacheStatsHolder.removeDimensions(List.of("A1", "B1")); - countDownLatch.countDown(); - }); - threads[2] = new Thread(() -> { - cacheStatsHolder.incrementMisses(List.of("A2", "B2")); - cacheStatsHolder.incrementMisses(List.of("A2", "B3")); + Thread[] threads = new Thread[numAValues + 1]; + CountDownLatch countDownLatch = new CountDownLatch(numAValues + 1); + Phaser phaser = new Phaser(numAValues + 2); + for (int i = 0; i < numAValues; i++) { + int finalI = i; + threads[i] = new Thread(() -> { + phaser.arriveAndAwaitAdvance(); + cacheStatsHolder.removeDimensions(List.of("A" + finalI, "B0")); + countDownLatch.countDown(); + }); + } + threads[numAValues] = new Thread(() -> { + phaser.arriveAndAwaitAdvance(); + cacheStatsHolder.removeDimensions(List.of("A0", "B1")); countDownLatch.countDown(); }); for (Thread thread : threads) { thread.start(); - // Add short sleep to ensure threads start their functions in order (so that incrementing doesn't happen before removal) - Thread.sleep(1); } + + phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - assertNull(getNode(List.of("A1", "B1"), cacheStatsHolder.getStatsRoot())); - assertNull(getNode(List.of("A1"), cacheStatsHolder.getStatsRoot())); - assertNotNull(getNode(List.of("A2", "B2"), cacheStatsHolder.getStatsRoot())); - assertEquals( - new ImmutableCacheStats(0, 1, 0, 0, 0), - getNode(List.of("A2", "B2"), cacheStatsHolder.getStatsRoot()).getImmutableStats() - ); - assertEquals( - new ImmutableCacheStats(1, 1, 0, 0, 0), - getNode(List.of("A2", "B3"), cacheStatsHolder.getStatsRoot()).getImmutableStats() - ); + + // intermediate node for A0 should be null + assertNull(getNode(List.of("A0"), cacheStatsHolder.getStatsRoot())); + + // leaf nodes for all B0 values should be null since they were removed + for (int indexA = 0; indexA < numAValues; indexA++) { + assertNull(getNode(List.of("A" + indexA, "B0"), cacheStatsHolder.getStatsRoot())); + } + + // leaf nodes for all B1 values, except (A0, B1), should not be null as they weren't removed, + // and the intermediate nodes A1 through A9 shouldn't be null as they have remaining children + for (int indexA = 1; indexA < numAValues; indexA++) { + DefaultCacheStatsHolder.Node b1LeafNode = getNode(List.of("A" + indexA, "B1"), cacheStatsHolder.getStatsRoot()); + assertNotNull(b1LeafNode); + assertEquals(new ImmutableCacheStats(1, 0, 0, 0, 0), b1LeafNode.getImmutableStats()); + DefaultCacheStatsHolder.Node intermediateLevelNode = getNode(List.of("A" + indexA), cacheStatsHolder.getStatsRoot()); + assertNotNull(intermediateLevelNode); + assertEquals(b1LeafNode.getImmutableStats(), intermediateLevelNode.getImmutableStats()); + } } /** From 406f8e2e23bcf1573cba209cf1d684d831f32c44 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 19 Jun 2024 16:51:38 -0700 Subject: [PATCH 2/4] rerun gradle Signed-off-by: Peter Alfonsi From 32e87e4c8bc46dff3ad1a60cbd33e454fb01ba3d Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 19 Jun 2024 17:04:51 -0700 Subject: [PATCH 3/4] Addressed andrross's comment Signed-off-by: Peter Alfonsi --- .../stats/DefaultCacheStatsHolderTests.java | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java b/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java index 4ae56f47996b9..8a59dd9d2d105 100644 --- a/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java +++ b/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java @@ -20,7 +20,6 @@ import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.Phaser; public class DefaultCacheStatsHolderTests extends OpenSearchTestCase { private final String storeName = "dummy_store"; @@ -150,27 +149,17 @@ public void testConcurrentRemoval() throws Exception { // and reflect only the stats for their B1 child. Thread[] threads = new Thread[numAValues + 1]; - CountDownLatch countDownLatch = new CountDownLatch(numAValues + 1); - Phaser phaser = new Phaser(numAValues + 2); for (int i = 0; i < numAValues; i++) { int finalI = i; - threads[i] = new Thread(() -> { - phaser.arriveAndAwaitAdvance(); - cacheStatsHolder.removeDimensions(List.of("A" + finalI, "B0")); - countDownLatch.countDown(); - }); + threads[i] = new Thread(() -> { cacheStatsHolder.removeDimensions(List.of("A" + finalI, "B0")); }); } - threads[numAValues] = new Thread(() -> { - phaser.arriveAndAwaitAdvance(); - cacheStatsHolder.removeDimensions(List.of("A0", "B1")); - countDownLatch.countDown(); - }); + threads[numAValues] = new Thread(() -> { cacheStatsHolder.removeDimensions(List.of("A0", "B1")); }); for (Thread thread : threads) { thread.start(); } - - phaser.arriveAndAwaitAdvance(); - countDownLatch.await(); + for (Thread thread : threads) { + thread.join(); + } // intermediate node for A0 should be null assertNull(getNode(List.of("A0"), cacheStatsHolder.getStatsRoot())); From 8dbea69c6721735ba99aa1c15f5354ed9ed846a7 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 20 Jun 2024 09:27:49 -0700 Subject: [PATCH 4/4] rerun gradle Signed-off-by: Peter Alfonsi