From 48e8c1a1aa8d8760b5c977e47db8e51f53c8face Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 10 Dec 2024 15:11:30 -0800 Subject: [PATCH] Addressed Ankit's comment Signed-off-by: Peter Alfonsi --- .../TieredSpilloverCacheStatsHolderTests.java | 65 ++++++++----------- .../cache/stats/DefaultCacheStatsHolder.java | 4 +- 2 files changed, 30 insertions(+), 39 deletions(-) diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java index 09273a0761663..4524e1592b005 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java @@ -12,6 +12,7 @@ import org.opensearch.common.cache.stats.CacheStats; import org.opensearch.common.cache.stats.DefaultCacheStatsHolder; import org.opensearch.common.cache.stats.ImmutableCacheStats; +import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.test.OpenSearchTestCase; import java.util.ArrayList; @@ -41,9 +42,7 @@ public void testAddAndGet() throws Exception { // test the value in the map is as expected for each distinct combination of values (all leaf nodes) for (List dimensionValues : expected.keySet()) { CacheStats expectedCounter = expected.get(dimensionValues); - ImmutableCacheStats actualStatsHolder = getNode(dimensionValues, cacheStatsHolder.getStatsRoot()).getImmutableStats(); - ImmutableCacheStats actualCacheStats = getNode(dimensionValues, cacheStatsHolder.getStatsRoot()).getImmutableStats(); - assertEquals(expectedCounter.immutableSnapshot(), actualStatsHolder); + ImmutableCacheStats actualCacheStats = getNodeStats(dimensionValues, cacheStatsHolder); assertEquals(expectedCounter.immutableSnapshot(), actualCacheStats); } @@ -55,7 +54,7 @@ public void testAddAndGet() throws Exception { || !diskTierEnabled; add(expectedTotal, other, countMissesAndEvictionsTowardsTotal); } - assertEquals(expectedTotal.immutableSnapshot(), cacheStatsHolder.getStatsRoot().getImmutableStats()); + assertEquals(expectedTotal.immutableSnapshot(), cacheStatsHolder.getImmutableCacheStatsHolder(null).getTotalStats()); } } @@ -88,8 +87,7 @@ public void testReset() throws Exception { 0 ); - DefaultCacheStatsHolder.Node node = getNode(dimensionValues, cacheStatsHolder.getStatsRoot()); - ImmutableCacheStats actual = node.getImmutableStats(); + ImmutableCacheStats actual = getNodeStats(dimensionValues, cacheStatsHolder); assertEquals(expectedTotal, actual); } } @@ -109,7 +107,7 @@ public void testDropStatsForDimensions() throws Exception { // disk tier is absent, count the heap ones) long originalHits = diskTierEnabled ? 2 * numNodes : numNodes; ImmutableCacheStats expectedTotal = new ImmutableCacheStats(originalHits, numNodes, numNodes, 0, 0); - assertEquals(expectedTotal, cacheStatsHolder.getStatsRoot().getImmutableStats()); + assertEquals(expectedTotal, cacheStatsHolder.getImmutableCacheStatsHolder(null).getTotalStats()); // When we invalidate A2, B2, we should lose the node for B2, but not B3 or A2. cacheStatsHolder.removeDimensions(List.of("A2", "B2")); @@ -118,25 +116,25 @@ public void testDropStatsForDimensions() throws Exception { // either case. long removedHitsPerRemovedNode = diskTierEnabled ? 2 : 1; expectedTotal = new ImmutableCacheStats(originalHits - removedHitsPerRemovedNode, numNodes - 1, numNodes - 1, 0, 0); - assertEquals(expectedTotal, cacheStatsHolder.getStatsRoot().getImmutableStats()); - assertNull(getNode(List.of("A2", "B2", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); - assertNull(getNode(List.of("A2", "B2", TIER_DIMENSION_VALUE_DISK), cacheStatsHolder.getStatsRoot())); - assertNull(getNode(List.of("A2", "B2"), cacheStatsHolder.getStatsRoot())); - assertNotNull(getNode(List.of("A2"), cacheStatsHolder.getStatsRoot())); - assertNotNull(getNode(List.of("A2", "B3", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); + assertEquals(expectedTotal, cacheStatsHolder.getImmutableCacheStatsHolder(null).getTotalStats()); + assertNull(getNodeStats(List.of("A2", "B2", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder)); + assertNull(getNodeStats(List.of("A2", "B2", TIER_DIMENSION_VALUE_DISK), cacheStatsHolder)); + assertNull(getNodeStats(List.of("A2", "B2"), cacheStatsHolder)); + assertNotNull(getNodeStats(List.of("A2"), cacheStatsHolder)); + assertNotNull(getNodeStats(List.of("A2", "B3", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder)); // When we invalidate A1, B1, we should lose the nodes for B1 and also A1, as it has no more children. cacheStatsHolder.removeDimensions(List.of("A1", "B1")); expectedTotal = new ImmutableCacheStats(originalHits - 2 * removedHitsPerRemovedNode, numNodes - 2, numNodes - 2, 0, 0); - assertEquals(expectedTotal, cacheStatsHolder.getStatsRoot().getImmutableStats()); - assertNull(getNode(List.of("A1", "B1", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); - assertNull(getNode(List.of("A1", "B1", TIER_DIMENSION_VALUE_DISK), cacheStatsHolder.getStatsRoot())); - assertNull(getNode(List.of("A1", "B1"), cacheStatsHolder.getStatsRoot())); - assertNull(getNode(List.of("A1"), cacheStatsHolder.getStatsRoot())); + assertEquals(expectedTotal, cacheStatsHolder.getImmutableCacheStatsHolder(null).getTotalStats()); + assertNull(getNodeStats(List.of("A1", "B1", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder)); + assertNull(getNodeStats(List.of("A1", "B1", TIER_DIMENSION_VALUE_DISK), cacheStatsHolder)); + assertNull(getNodeStats(List.of("A1", "B1"), cacheStatsHolder)); + assertNull(getNodeStats(List.of("A1"), cacheStatsHolder)); // When we invalidate the last node, all nodes should be deleted except the root node cacheStatsHolder.removeDimensions(List.of("A2", "B3")); - assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), cacheStatsHolder.getStatsRoot().getImmutableStats()); + assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), cacheStatsHolder.getImmutableCacheStatsHolder(null).getTotalStats()); // assertEquals(0, cacheStatsHolder.getStatsRoot().getChildren().size()); } } @@ -188,22 +186,20 @@ public void testConcurrentRemoval() throws Exception { } // intermediate node for A0 should be null - assertNull(getNode(List.of("A0"), cacheStatsHolder.getStatsRoot())); + assertNull(getNodeStats(List.of("A0"), cacheStatsHolder)); // 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())); + assertNull(getNodeStats(List.of("A" + indexA, "B0"), cacheStatsHolder)); } // 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(2, 1, 1, 0, 0), b1LeafNode.getImmutableStats()); - DefaultCacheStatsHolder.Node intermediateLevelNode = getNode(List.of("A" + indexA), cacheStatsHolder.getStatsRoot()); - assertNotNull(intermediateLevelNode); - assertEquals(b1LeafNode.getImmutableStats(), intermediateLevelNode.getImmutableStats()); + ImmutableCacheStats b1LeafNodeStats = getNodeStats(List.of("A" + indexA, "B1"), cacheStatsHolder); + assertEquals(new ImmutableCacheStats(2, 1, 1, 0, 0), b1LeafNodeStats); + ImmutableCacheStats intermediateLevelNodeStats = getNodeStats(List.of("A" + indexA), cacheStatsHolder); + assertEquals(b1LeafNodeStats, intermediateLevelNodeStats); } } @@ -226,18 +222,13 @@ static void setupRemovalTest( } /** - * Returns the node found by following these dimension values down from the root node. + * Returns the stats from node found by following these dimension values down from the root node. * Returns null if no such node exists. */ - static DefaultCacheStatsHolder.Node getNode(List dimensionValues, DefaultCacheStatsHolder.Node root) { - DefaultCacheStatsHolder.Node current = root; - for (String dimensionValue : dimensionValues) { - current = current.getChildren().get(dimensionValue); - if (current == null) { - return null; - } - } - return current; + static ImmutableCacheStats getNodeStats(List dimensionValues, DefaultCacheStatsHolder holder) { + String[] levels = holder.getDimensionNames().toArray(new String[0]); + ImmutableCacheStatsHolder immutableHolder = holder.getImmutableCacheStatsHolder(levels); + return immutableHolder.getStatsForDimensionValues(dimensionValues); } static Map, CacheStats> populateStats( diff --git a/server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java index 7434283ff6f41..f83b812af8299 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java @@ -210,14 +210,14 @@ protected ImmutableCacheStats removeDimensionsHelper(List dimensionValue return statsToDecrement; } - public Node getStatsRoot() { + Node getStatsRoot() { return statsRoot; } /** * Nodes that make up the tree in the stats holder. */ - public static class Node { + protected static class Node { private final String dimensionValue; // Map from dimensionValue to the DimensionNode for that dimension value. final Map children;