Skip to content

Commit 0eb39ae

Browse files
peteralfonsiPeter Alfonsi
and
Peter Alfonsi
authored
Fix flaky DefaultCacheStatsHolderTests (#14462)
Signed-off-by: Peter Alfonsi <petealft@amazon.com> Co-authored-by: Peter Alfonsi <petealft@amazon.com>
1 parent d320f36 commit 0eb39ae

File tree

1 file changed

+42
-33
lines changed

1 file changed

+42
-33
lines changed

server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java

+42-33
Original file line numberDiff line numberDiff line change
@@ -127,49 +127,58 @@ public void testCount() throws Exception {
127127
}
128128

129129
public void testConcurrentRemoval() throws Exception {
130-
List<String> dimensionNames = List.of("dim1", "dim2");
130+
List<String> dimensionNames = List.of("A", "B");
131131
DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames, storeName);
132132

133133
// Create stats for the following dimension sets
134-
List<List<String>> populatedStats = List.of(List.of("A1", "B1"), List.of("A2", "B2"), List.of("A2", "B3"));
134+
List<List<String>> populatedStats = new ArrayList<>();
135+
int numAValues = 10;
136+
int numBValues = 2;
137+
for (int indexA = 0; indexA < numAValues; indexA++) {
138+
for (int indexB = 0; indexB < numBValues; indexB++) {
139+
populatedStats.add(List.of("A" + indexA, "B" + indexB));
140+
}
141+
}
135142
for (List<String> dims : populatedStats) {
136143
cacheStatsHolder.incrementHits(dims);
137144
}
138145

139-
// 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).
140-
141-
Thread[] threads = new Thread[3];
142-
CountDownLatch countDownLatch = new CountDownLatch(3);
143-
threads[0] = new Thread(() -> {
144-
cacheStatsHolder.removeDimensions(List.of("A2", "B2"));
145-
countDownLatch.countDown();
146-
});
147-
threads[1] = new Thread(() -> {
148-
cacheStatsHolder.removeDimensions(List.of("A1", "B1"));
149-
countDownLatch.countDown();
150-
});
151-
threads[2] = new Thread(() -> {
152-
cacheStatsHolder.incrementMisses(List.of("A2", "B2"));
153-
cacheStatsHolder.incrementMisses(List.of("A2", "B3"));
154-
countDownLatch.countDown();
155-
});
146+
// Remove a subset of the dimensions concurrently.
147+
// Remove both (A0, B0), and (A0, B1), so we expect the intermediate node for A0 to be null afterwards.
148+
// For all the others, remove only the B0 value. Then we expect the intermediate nodes for A1 through A9 to be present
149+
// and reflect only the stats for their B1 child.
150+
151+
Thread[] threads = new Thread[numAValues + 1];
152+
for (int i = 0; i < numAValues; i++) {
153+
int finalI = i;
154+
threads[i] = new Thread(() -> { cacheStatsHolder.removeDimensions(List.of("A" + finalI, "B0")); });
155+
}
156+
threads[numAValues] = new Thread(() -> { cacheStatsHolder.removeDimensions(List.of("A0", "B1")); });
156157
for (Thread thread : threads) {
157158
thread.start();
158-
// Add short sleep to ensure threads start their functions in order (so that incrementing doesn't happen before removal)
159-
Thread.sleep(1);
160159
}
161-
countDownLatch.await();
162-
assertNull(getNode(List.of("A1", "B1"), cacheStatsHolder.getStatsRoot()));
163-
assertNull(getNode(List.of("A1"), cacheStatsHolder.getStatsRoot()));
164-
assertNotNull(getNode(List.of("A2", "B2"), cacheStatsHolder.getStatsRoot()));
165-
assertEquals(
166-
new ImmutableCacheStats(0, 1, 0, 0, 0),
167-
getNode(List.of("A2", "B2"), cacheStatsHolder.getStatsRoot()).getImmutableStats()
168-
);
169-
assertEquals(
170-
new ImmutableCacheStats(1, 1, 0, 0, 0),
171-
getNode(List.of("A2", "B3"), cacheStatsHolder.getStatsRoot()).getImmutableStats()
172-
);
160+
for (Thread thread : threads) {
161+
thread.join();
162+
}
163+
164+
// intermediate node for A0 should be null
165+
assertNull(getNode(List.of("A0"), cacheStatsHolder.getStatsRoot()));
166+
167+
// leaf nodes for all B0 values should be null since they were removed
168+
for (int indexA = 0; indexA < numAValues; indexA++) {
169+
assertNull(getNode(List.of("A" + indexA, "B0"), cacheStatsHolder.getStatsRoot()));
170+
}
171+
172+
// leaf nodes for all B1 values, except (A0, B1), should not be null as they weren't removed,
173+
// and the intermediate nodes A1 through A9 shouldn't be null as they have remaining children
174+
for (int indexA = 1; indexA < numAValues; indexA++) {
175+
DefaultCacheStatsHolder.Node b1LeafNode = getNode(List.of("A" + indexA, "B1"), cacheStatsHolder.getStatsRoot());
176+
assertNotNull(b1LeafNode);
177+
assertEquals(new ImmutableCacheStats(1, 0, 0, 0, 0), b1LeafNode.getImmutableStats());
178+
DefaultCacheStatsHolder.Node intermediateLevelNode = getNode(List.of("A" + indexA), cacheStatsHolder.getStatsRoot());
179+
assertNotNull(intermediateLevelNode);
180+
assertEquals(b1LeafNode.getImmutableStats(), intermediateLevelNode.getImmutableStats());
181+
}
173182
}
174183

175184
/**

0 commit comments

Comments
 (0)