From 668f7d65eff628e43c3556e1bbffa5d2e79c62b0 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 24 Oct 2024 16:19:22 -0700 Subject: [PATCH 01/12] added draft tests for tsc stats holder Signed-off-by: Peter Alfonsi --- .../TieredSpilloverCacheStatsHolderTests.java | 351 ++++++++++++++++++ .../common/cache/stats/CacheStats.java | 10 +- .../cache/stats/DefaultCacheStatsHolder.java | 10 +- 3 files changed, 361 insertions(+), 10 deletions(-) create mode 100644 modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java 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 new file mode 100644 index 0000000000000..b2841ef30d0c4 --- /dev/null +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java @@ -0,0 +1,351 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.cache.common.tier; + +import org.opensearch.common.Randomness; +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.common.metrics.CounterMetric; +import org.opensearch.test.OpenSearchTestCase; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Random; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; + +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; + +public class TieredSpilloverCacheStatsHolderTests extends OpenSearchTestCase { + // TODO: these are directly copied from DefaultCacheStatsHolderTests for now with tweaks to make it work + // Also made a bunch of things public for debug/testing only + + public void testAddAndGet() throws Exception { + List dimensionNames = List.of("dim1", "dim2"); //List.of("dim1", "dim2", "dim3", "dim4"); + TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); + Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 3); + Map, CacheStats> expected = populateStats( + cacheStatsHolder, + usedDimensionValues, + 10000, + 10 + ); + + // 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); + assertEquals(expectedCounter.immutableSnapshot(), actualCacheStats); + } + + // Check overall total matches + CacheStats expectedTotal = new CacheStats(); + for (List dims : expected.keySet()) { + CacheStats other = expected.get(dims); + boolean isHeapTier = dims.get(dims.size()-1).equals(TIER_DIMENSION_VALUE_ON_HEAP); + //System.out.println("is heap tier: " + isHeapTier); + add(expectedTotal, other, isHeapTier); + //expectedTotal.add(expected.get(dims)); + } + ImmutableCacheStats total = cacheStatsHolder.getStatsRoot().getImmutableStats(); + ImmutableCacheStatsHolder finalTree = cacheStatsHolder.getImmutableCacheStatsHolder(null); + assertEquals(expectedTotal.immutableSnapshot(), cacheStatsHolder.getStatsRoot().getImmutableStats()); + + // Check sum of children stats are correct + //assertSumOfChildrenStats(cacheStatsHolder.getStatsRoot()); + } + + private void add(CacheStats orig, CacheStats other, boolean otherIsHeap) { + // Add other to orig, accounting for whether other is from the heap or disk tier + long misses; + if (otherIsHeap) { + misses = 0; + } else { + misses = other.getMisses(); + } + CacheStats modifiedOther = new CacheStats(other.getHits(), misses, other.getEvictions(), other.getSizeInBytes(), other.getItems()); + orig.add(modifiedOther); + } + + public void testReset() throws Exception { + List dimensionNames = List.of("dim1", "dim2"); + TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); + Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10); + Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 100, 10); + + cacheStatsHolder.reset(); + + for (List dimensionValues : expected.keySet()) { + CacheStats originalCounter = expected.get(dimensionValues); + originalCounter.sizeInBytes = new CounterMetric(); + originalCounter.items = new CounterMetric(); + + DefaultCacheStatsHolder.Node node = getNode(dimensionValues, cacheStatsHolder.getStatsRoot()); + ImmutableCacheStats actual = node.getImmutableStats(); + assertEquals(originalCounter.immutableSnapshot(), actual); + } + } + + public void testDropStatsForDimensions() throws Exception { + List dimensionNames = List.of("dim1", "dim2"); + TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); + + // Create stats for the following dimension sets + List> populatedStats = List.of(List.of("A1", "B1", TIER_DIMENSION_VALUE_ON_HEAP), List.of("A2", "B2", TIER_DIMENSION_VALUE_ON_HEAP), List.of("A2", "B3", TIER_DIMENSION_VALUE_ON_HEAP)); + for (List dims : populatedStats) { + cacheStatsHolder.incrementHits(dims); + } + + assertEquals(3, cacheStatsHolder.getStatsRoot().getImmutableStats().getHits()); + + // When we invalidate A2, B2, we should lose the node for B2, but not B3 or A2. + + cacheStatsHolder.removeDimensions(List.of("A2", "B2", TIER_DIMENSION_VALUE_ON_HEAP)); + + assertEquals(2, cacheStatsHolder.getStatsRoot().getImmutableStats().getHits()); + assertNull(getNode(List.of("A2", "B2", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); + assertNotNull(getNode(List.of("A2"), cacheStatsHolder.getStatsRoot())); + assertNotNull(getNode(List.of("A2", "B3", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); + + // 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", TIER_DIMENSION_VALUE_ON_HEAP)); + + assertEquals(1, cacheStatsHolder.getStatsRoot().getImmutableStats().getHits()); + assertNull(getNode(List.of("A1", "B1", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); + assertNull(getNode(List.of("A1", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); + + // When we invalidate the last node, all nodes should be deleted except the root node + + cacheStatsHolder.removeDimensions(List.of("A2", "B3", TIER_DIMENSION_VALUE_ON_HEAP)); + assertEquals(0, cacheStatsHolder.getStatsRoot().getImmutableStats().getHits()); + assertEquals(0, cacheStatsHolder.getStatsRoot().children.size()); + } + + public void testCount() throws Exception { + List dimensionNames = List.of("dim1", "dim2"); + TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); + Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10); + Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 100, 10); + + long expectedCount = 0L; + for (CacheStats counter : expected.values()) { + expectedCount += counter.getItems(); + } + assertEquals(expectedCount, cacheStatsHolder.count()); + } + + public void testConcurrentRemoval() throws Exception { + List dimensionNames = List.of("A", "B"); + TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); + + // Create stats for the following dimension sets + 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 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[numAValues + 1]; + for (int i = 0; i < numAValues; i++) { + int finalI = i; + threads[i] = new Thread(() -> { cacheStatsHolder.removeDimensions(List.of("A" + finalI, "B0")); }); + } + threads[numAValues] = new Thread(() -> { cacheStatsHolder.removeDimensions(List.of("A0", "B1")); }); + for (Thread thread : threads) { + thread.start(); + } + for (Thread thread : threads) { + thread.join(); + } + + // 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()); + } + } + + /** + * Returns the 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 Map, CacheStats> populateStats( + TieredSpilloverCacheStatsHolder cacheStatsHolder, + Map> usedDimensionValues, + int numDistinctValuePairs, + int numRepetitionsPerValue + ) throws InterruptedException { + return populateStats(List.of(cacheStatsHolder), usedDimensionValues, numDistinctValuePairs, numRepetitionsPerValue); + } + + static Map, CacheStats> populateStats( + List cacheStatsHolders, + Map> usedDimensionValues, + int numDistinctValuePairs, + int numRepetitionsPerValue + ) throws InterruptedException { + for (TieredSpilloverCacheStatsHolder statsHolder : cacheStatsHolders) { + assertEquals(cacheStatsHolders.get(0).getDimensionNames(), statsHolder.getDimensionNames()); + } + Map, CacheStats> expected = new ConcurrentHashMap<>(); + Thread[] threads = new Thread[numDistinctValuePairs]; + CountDownLatch countDownLatch = new CountDownLatch(numDistinctValuePairs); + Random rand = Randomness.get(); + List> dimensionsForThreads = new ArrayList<>(); + for (int i = 0; i < numDistinctValuePairs; i++) { + dimensionsForThreads.add(getRandomDimList(cacheStatsHolders.get(0).getDimensionNames(), usedDimensionValues, true, rand)); + int finalI = i; + threads[i] = new Thread(() -> { + Random threadRand = Randomness.get(); + List dimensions = dimensionsForThreads.get(finalI); + expected.computeIfAbsent(dimensions, (key) -> new CacheStats()); + for (TieredSpilloverCacheStatsHolder cacheStatsHolder : cacheStatsHolders) { + for (int j = 0; j < numRepetitionsPerValue; j++) { + CacheStats statsToInc = new CacheStats( + threadRand.nextInt(10), + threadRand.nextInt(10), + threadRand.nextInt(10), + threadRand.nextInt(5000), + threadRand.nextInt(10) + ); + expected.get(dimensions).hits.inc(statsToInc.getHits()); + expected.get(dimensions).misses.inc(statsToInc.getMisses()); + expected.get(dimensions).evictions.inc(statsToInc.getEvictions()); + expected.get(dimensions).sizeInBytes.inc(statsToInc.getSizeInBytes()); + expected.get(dimensions).items.inc(statsToInc.getItems()); + populateStatsHolderFromStatsValueMap(cacheStatsHolder, Map.of(dimensions, statsToInc)); + } + } + countDownLatch.countDown(); + }); + } + for (Thread thread : threads) { + thread.start(); + } + countDownLatch.await(); + return expected; + } + + private static List getRandomDimList( + List dimensionNames, + Map> usedDimensionValues, + boolean pickValueForAllDims, + Random rand + ) { + List result = new ArrayList<>(); + for (String dimName : dimensionNames) { + if (pickValueForAllDims || rand.nextBoolean()) { // if pickValueForAllDims, always pick a value for each dimension, otherwise do + // so 50% of the time + int index = between(0, usedDimensionValues.get(dimName).size() - 1); + result.add(usedDimensionValues.get(dimName).get(index)); + } + } + return result; + } + + static Map> getUsedDimensionValues(TieredSpilloverCacheStatsHolder cacheStatsHolder, int numValuesPerDim) { + Map> usedDimensionValues = new HashMap<>(); + for (int i = 0; i < cacheStatsHolder.getDimensionNames().size() - 1; i++) { // Have to handle final tier dimension separately + List values = new ArrayList<>(); + for (int j = 0; j < numValuesPerDim; j++) { + values.add(UUID.randomUUID().toString()); + } + usedDimensionValues.put(cacheStatsHolder.getDimensionNames().get(i), values); + } + List tierValues = List.of(TIER_DIMENSION_VALUE_ON_HEAP, TIER_DIMENSION_VALUE_DISK); + usedDimensionValues.put(TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME, tierValues); + return usedDimensionValues; + } + + private void assertSumOfChildrenStats(DefaultCacheStatsHolder.Node current) { + if (!current.children.isEmpty()) { + CacheStats expectedTotal = new CacheStats(); + for (DefaultCacheStatsHolder.Node child : current.children.values()) { + expectedTotal.add(child.getImmutableStats()); + } + assertEquals(expectedTotal.immutableSnapshot(), current.getImmutableStats()); + for (DefaultCacheStatsHolder.Node child : current.children.values()) { + assertSumOfChildrenStats(child); + } + } + } + + public static void populateStatsHolderFromStatsValueMap( + TieredSpilloverCacheStatsHolder cacheStatsHolder, + Map, CacheStats> statsMap + ) { + for (Map.Entry, CacheStats> entry : statsMap.entrySet()) { + CacheStats stats = entry.getValue(); + List dims = entry.getKey(); + for (int i = 0; i < stats.getHits(); i++) { + cacheStatsHolder.incrementHits(dims); + } + for (int i = 0; i < stats.getMisses(); i++) { + cacheStatsHolder.incrementMisses(dims); + } + for (int i = 0; i < stats.getEvictions(); i++) { + // TODO: for now include all disk evictions in total + boolean includeInTotal = dims.get(dims.size()-1).equals(TIER_DIMENSION_VALUE_DISK); + //System.out.println("Include in total = " + includeInTotal); + cacheStatsHolder.incrementEvictions(dims, true); + } + cacheStatsHolder.incrementSizeInBytes(dims, stats.getSizeInBytes()); + for (int i = 0; i < stats.getItems(); i++) { + cacheStatsHolder.incrementItems(dims); + } + } + } +} diff --git a/server/src/main/java/org/opensearch/common/cache/stats/CacheStats.java b/server/src/main/java/org/opensearch/common/cache/stats/CacheStats.java index 93fa1ff7fcddf..5312961bb7591 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/CacheStats.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/CacheStats.java @@ -16,11 +16,11 @@ * A mutable class containing the 5 live metrics tracked by a StatsHolder object. */ public class CacheStats { - CounterMetric hits; - CounterMetric misses; - CounterMetric evictions; - CounterMetric sizeInBytes; - CounterMetric items; + public CounterMetric hits; + public CounterMetric misses; + public CounterMetric evictions; + public CounterMetric sizeInBytes; + public CounterMetric items; public CacheStats(long hits, long misses, long evictions, long sizeInBytes, long items) { this.hits = new CounterMetric(); 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 ea92c8e81b8f0..e9ec22b1afe99 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 @@ -209,17 +209,17 @@ private ImmutableCacheStats removeDimensionsHelper(List dimensionValues, } // pkg-private for testing - Node getStatsRoot() { + public Node getStatsRoot() { return statsRoot; } /** * Nodes that make up the tree in the stats holder. */ - protected static class Node { + public static class Node { private final String dimensionValue; // Map from dimensionValue to the DimensionNode for that dimension value. - final Map children; + public final Map children; // The stats for this node. If a leaf node, corresponds to the stats for this combination of dimensions; if not, // contains the sum of its children's stats. private CacheStats stats; @@ -241,7 +241,7 @@ public String getDimensionValue() { return dimensionValue; } - protected Map getChildren() { + public Map getChildren() { // We can safely iterate over ConcurrentHashMap without worrying about thread issues. return children; } @@ -280,7 +280,7 @@ long getEntries() { return this.stats.getItems(); } - ImmutableCacheStats getImmutableStats() { + public ImmutableCacheStats getImmutableStats() { return this.stats.immutableSnapshot(); } From e3771706852238ee46179a1a5dfde9b364b19e27 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 1 Nov 2024 11:30:35 -0700 Subject: [PATCH 02/12] first draft tsc stats bugfix Signed-off-by: Peter Alfonsi --- .../tier/TieredSpilloverCacheStatsIT.java | 6 +-- .../common/tier/TieredSpilloverCache.java | 5 ++- .../TieredSpilloverCacheStatsHolder.java | 44 +++++++++++++++++-- .../tier/TieredSpilloverCacheTests.java | 6 +-- .../cache/stats/DefaultCacheStatsHolder.java | 2 +- 5 files changed, 51 insertions(+), 12 deletions(-) rename modules/cache-common/src/main/java/org/opensearch/{cache/common/tier => common/cache/stats}/TieredSpilloverCacheStatsHolder.java (78%) diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java index fe6bd7050a8f3..72dfd387f7a17 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java @@ -37,9 +37,9 @@ import java.util.Map; import java.util.concurrent.TimeUnit; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse; diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java index ab5335ca0ca66..752b0c452fab5 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java @@ -21,6 +21,7 @@ import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.policy.CachedQueryResult; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; +import org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder; import org.opensearch.common.cache.store.config.CacheConfig; import org.opensearch.common.collect.Tuple; import org.opensearch.common.settings.Setting; @@ -53,8 +54,8 @@ import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java similarity index 78% rename from modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java rename to modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java index b40724430454b..efcfc64509b1a 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java @@ -6,8 +6,9 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.stats; +import org.opensearch.cache.common.tier.TieredSpilloverCache; import org.opensearch.common.cache.stats.DefaultCacheStatsHolder; import java.util.ArrayList; @@ -65,7 +66,7 @@ private static List getDimensionNamesWithTier(List dimensionName /** * Add tierValue to the end of a copy of the initial dimension values, so they can appropriately be used in this stats holder. */ - List getDimensionsWithTierValue(List initialDimensions, String tierValue) { + public List getDimensionsWithTierValue(List initialDimensions, String tierValue) { List result = new ArrayList<>(initialDimensions); result.add(tierValue); return result; @@ -164,7 +165,44 @@ public void decrementItems(List dimensionValues) { super.decrementItems(dimensionValues); } - void setDiskCacheEnabled(boolean diskCacheEnabled) { + public void setDiskCacheEnabled(boolean diskCacheEnabled) { this.diskCacheEnabled = diskCacheEnabled; } + + @Override + public void removeDimensions(List dimensionValues) { + assert dimensionValues.size() == dimensionNames.size() - 1 : "Must specify a value for every dimension except tier when removing from StatsHolder"; + // As we are removing nodes from the tree, obtain the lock + lock.lock(); + try { + removeDimensionsHelper(dimensionValues, getStatsRoot(), 0); + } finally { + lock.unlock(); + } + } + + private ImmutableCacheStats removeDimensionsHelper(List dimensionValues, Node node, int depth) { + if (depth == dimensionValues.size() - 1) { // TODO: subtract 1 bc we don't want to subtract from tier nodes, just delete them + // Delete this node's children (which represent individual tiers) TODO: is this needed or does GC get it anyway? + for (Node child : node.getChildren().values()) { + // TODO: iteration issues? + node.children.remove(child.getDimensionValue()); + } + // Pass up a snapshot of the original stats to avoid issues when the original is decremented by other fn invocations + return node.getImmutableStats(); + } + Node child = node.getChild(dimensionValues.get(depth)); + if (child == null) { + return null; + } + ImmutableCacheStats statsToDecrement = removeDimensionsHelper(dimensionValues, child, depth + 1); + if (statsToDecrement != null) { + // The removal took place, decrement values and remove this node from its parent if it's now empty + node.decrementBySnapshot(statsToDecrement); + if (child.getChildren().isEmpty()) { + node.children.remove(child.getDimensionValue()); + } + } + return statsToDecrement; + } } diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java index 1215a2130ac2d..d55f93399cbc9 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java @@ -61,9 +61,9 @@ import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; import static org.opensearch.common.cache.store.settings.OpenSearchOnHeapCacheSettings.MAXIMUM_SIZE_IN_BYTES_KEY; 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 e9ec22b1afe99..6f0d9faa0158f 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 @@ -40,7 +40,7 @@ public class DefaultCacheStatsHolder implements CacheStatsHolder { private final Node statsRoot; // To avoid sync problems, obtain a lock before creating or removing nodes in the stats tree. // No lock is needed to edit stats on existing nodes. - private final Lock lock = new ReentrantLock(); + protected final Lock lock = new ReentrantLock(); // The name of the cache type using these stats private final String storeName; From cd04540cac874350e793aa20d1762753d54e8701 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 11:55:11 -0800 Subject: [PATCH 03/12] Complete tests Signed-off-by: Peter Alfonsi --- .../common/tier/TieredSpilloverCache.java | 4 +- .../TieredSpilloverCacheStatsHolder.java | 8 +- .../tier/TieredSpilloverCacheTests.java | 4 +- .../TieredSpilloverCacheStatsHolderTests.java | 251 ++++++++++-------- .../common/cache/stats/CacheStats.java | 10 +- .../cache/stats/DefaultCacheStatsHolder.java | 8 +- 6 files changed, 150 insertions(+), 135 deletions(-) rename modules/cache-common/src/test/java/org/opensearch/{cache/common/tier => common/cache/stats}/TieredSpilloverCacheStatsHolderTests.java (54%) diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java index 752b0c452fab5..6cecdcab8fb33 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java @@ -54,10 +54,10 @@ import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; /** * This cache spillover the evicted items from heap tier to disk tier. All the new items are first cached on heap diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java index efcfc64509b1a..a9151c9a7a3ee 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java @@ -9,7 +9,6 @@ package org.opensearch.common.cache.stats; import org.opensearch.cache.common.tier.TieredSpilloverCache; -import org.opensearch.common.cache.stats.DefaultCacheStatsHolder; import java.util.ArrayList; import java.util.List; @@ -171,7 +170,8 @@ public void setDiskCacheEnabled(boolean diskCacheEnabled) { @Override public void removeDimensions(List dimensionValues) { - assert dimensionValues.size() == dimensionNames.size() - 1 : "Must specify a value for every dimension except tier when removing from StatsHolder"; + assert dimensionValues.size() == dimensionNames.size() - 1 + : "Must specify a value for every dimension except tier when removing from StatsHolder"; // As we are removing nodes from the tree, obtain the lock lock.lock(); try { @@ -182,8 +182,8 @@ public void removeDimensions(List dimensionValues) { } private ImmutableCacheStats removeDimensionsHelper(List dimensionValues, Node node, int depth) { - if (depth == dimensionValues.size() - 1) { // TODO: subtract 1 bc we don't want to subtract from tier nodes, just delete them - // Delete this node's children (which represent individual tiers) TODO: is this needed or does GC get it anyway? + if (depth == dimensionValues.size()) { // dimensionValues passed in doesn't include the tier dimension + // Manually delete this node's children (which represent individual tiers) TODO: is this needed or does GC get it anyway? for (Node child : node.getChildren().values()) { // TODO: iteration issues? node.children.remove(child.getDimensionValue()); diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java index d55f93399cbc9..051aa0354babe 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java @@ -61,11 +61,11 @@ import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; +import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; +import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; -import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; -import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; import static org.opensearch.common.cache.store.settings.OpenSearchOnHeapCacheSettings.MAXIMUM_SIZE_IN_BYTES_KEY; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; 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/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java similarity index 54% rename from modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java rename to modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java index b2841ef30d0c4..c7fe173113e5f 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java @@ -6,13 +6,9 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.stats; import org.opensearch.common.Randomness; -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.common.metrics.CounterMetric; import org.opensearch.test.OpenSearchTestCase; @@ -25,73 +21,60 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; public class TieredSpilloverCacheStatsHolderTests extends OpenSearchTestCase { - // TODO: these are directly copied from DefaultCacheStatsHolderTests for now with tweaks to make it work - // Also made a bunch of things public for debug/testing only + // These are modified from DefaultCacheStatsHolderTests.java to account for the tiers. Because we can't add a dependency on server.test, + // we can't reuse the same code. public void testAddAndGet() throws Exception { - List dimensionNames = List.of("dim1", "dim2"); //List.of("dim1", "dim2", "dim3", "dim4"); - TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); - Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 3); - Map, CacheStats> expected = populateStats( - cacheStatsHolder, - usedDimensionValues, - 10000, - 10 - ); - - // 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); - assertEquals(expectedCounter.immutableSnapshot(), actualCacheStats); - } + for (boolean diskTierEnabled : List.of(true, false)) { + List dimensionNames = List.of("dim1", "dim2", "dim3", "dim4"); + TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, diskTierEnabled); + Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10, diskTierEnabled); + Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 1000, 10, diskTierEnabled); + + // 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); + assertEquals(expectedCounter.immutableSnapshot(), actualCacheStats); + } - // Check overall total matches - CacheStats expectedTotal = new CacheStats(); - for (List dims : expected.keySet()) { - CacheStats other = expected.get(dims); - boolean isHeapTier = dims.get(dims.size()-1).equals(TIER_DIMENSION_VALUE_ON_HEAP); - //System.out.println("is heap tier: " + isHeapTier); - add(expectedTotal, other, isHeapTier); - //expectedTotal.add(expected.get(dims)); + // Check overall total matches + CacheStats expectedTotal = new CacheStats(); + for (List dims : expected.keySet()) { + CacheStats other = expected.get(dims); + boolean countMissesAndEvictionsTowardsTotal = dims.get(dims.size() - 1).equals(TIER_DIMENSION_VALUE_DISK) + || !diskTierEnabled; + add(expectedTotal, other, countMissesAndEvictionsTowardsTotal); + } + assertEquals(expectedTotal.immutableSnapshot(), cacheStatsHolder.getStatsRoot().getImmutableStats()); } - ImmutableCacheStats total = cacheStatsHolder.getStatsRoot().getImmutableStats(); - ImmutableCacheStatsHolder finalTree = cacheStatsHolder.getImmutableCacheStatsHolder(null); - assertEquals(expectedTotal.immutableSnapshot(), cacheStatsHolder.getStatsRoot().getImmutableStats()); - - // Check sum of children stats are correct - //assertSumOfChildrenStats(cacheStatsHolder.getStatsRoot()); } - private void add(CacheStats orig, CacheStats other, boolean otherIsHeap) { - // Add other to orig, accounting for whether other is from the heap or disk tier - long misses; - if (otherIsHeap) { - misses = 0; - } else { + private void add(CacheStats original, CacheStats other, boolean countMissesAndEvictionsTowardsTotal) { + // Add other to original, accounting for whether other is from the heap or disk tier + long misses = 0; + long evictions = 0; + if (countMissesAndEvictionsTowardsTotal) { misses = other.getMisses(); + evictions = other.getEvictions(); } - CacheStats modifiedOther = new CacheStats(other.getHits(), misses, other.getEvictions(), other.getSizeInBytes(), other.getItems()); - orig.add(modifiedOther); + CacheStats modifiedOther = new CacheStats(other.getHits(), misses, evictions, other.getSizeInBytes(), other.getItems()); + original.add(modifiedOther); } public void testReset() throws Exception { List dimensionNames = List.of("dim1", "dim2"); TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); - Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10); - Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 100, 10); + Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10, true); + Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 100, 10, true); cacheStatsHolder.reset(); - for (List dimensionValues : expected.keySet()) { CacheStats originalCounter = expected.get(dimensionValues); originalCounter.sizeInBytes = new CounterMetric(); @@ -105,45 +88,56 @@ public void testReset() throws Exception { public void testDropStatsForDimensions() throws Exception { List dimensionNames = List.of("dim1", "dim2"); - TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); - // Create stats for the following dimension sets - List> populatedStats = List.of(List.of("A1", "B1", TIER_DIMENSION_VALUE_ON_HEAP), List.of("A2", "B2", TIER_DIMENSION_VALUE_ON_HEAP), List.of("A2", "B3", TIER_DIMENSION_VALUE_ON_HEAP)); - for (List dims : populatedStats) { - cacheStatsHolder.incrementHits(dims); + List> statsToPopulate = List.of(List.of("A1", "B1"), List.of("A2", "B2"), List.of("A2", "B3")); + for (boolean diskTierEnabled : List.of(true, false)) { + TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, diskTierEnabled); + setupRemovalTest(cacheStatsHolder, statsToPopulate, diskTierEnabled); + + // Check the resulting total is correct. + int numNodes = statsToPopulate.size(); // Number of distinct sets of dimensions (not including tiers) + // If disk tier is enabled, we expect hits to be 2 * numNodes (1 heap + 1 disk per combination of dims), otherwise 1 * numNodes. + // Misses and evictions should be 1 * numNodes in either case (if disk tier is present, count only the disk misses/evictions, if + // 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()); + + // When we invalidate A2, B2, we should lose the node for B2, but not B3 or A2. + cacheStatsHolder.removeDimensions(List.of("A2", "B2")); + + // We expect hits to go down by 2 (1 heap + 1 disk) if disk is enabled, and 1 otherwise. Evictions/misses should go down by 1 in + // 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())); + + // 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())); + + // 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(0, cacheStatsHolder.getStatsRoot().children.size()); } - - assertEquals(3, cacheStatsHolder.getStatsRoot().getImmutableStats().getHits()); - - // When we invalidate A2, B2, we should lose the node for B2, but not B3 or A2. - - cacheStatsHolder.removeDimensions(List.of("A2", "B2", TIER_DIMENSION_VALUE_ON_HEAP)); - - assertEquals(2, cacheStatsHolder.getStatsRoot().getImmutableStats().getHits()); - assertNull(getNode(List.of("A2", "B2", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); - assertNotNull(getNode(List.of("A2"), cacheStatsHolder.getStatsRoot())); - assertNotNull(getNode(List.of("A2", "B3", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); - - // 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", TIER_DIMENSION_VALUE_ON_HEAP)); - - assertEquals(1, cacheStatsHolder.getStatsRoot().getImmutableStats().getHits()); - assertNull(getNode(List.of("A1", "B1", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); - assertNull(getNode(List.of("A1", TIER_DIMENSION_VALUE_ON_HEAP), cacheStatsHolder.getStatsRoot())); - - // When we invalidate the last node, all nodes should be deleted except the root node - - cacheStatsHolder.removeDimensions(List.of("A2", "B3", TIER_DIMENSION_VALUE_ON_HEAP)); - assertEquals(0, cacheStatsHolder.getStatsRoot().getImmutableStats().getHits()); - assertEquals(0, cacheStatsHolder.getStatsRoot().children.size()); } public void testCount() throws Exception { List dimensionNames = List.of("dim1", "dim2"); TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); - Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10); - Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 100, 10); + Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10, true); + Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 100, 10, true); long expectedCount = 0L; for (CacheStats counter : expected.values()) { @@ -157,17 +151,15 @@ public void testConcurrentRemoval() throws Exception { TieredSpilloverCacheStatsHolder cacheStatsHolder = new TieredSpilloverCacheStatsHolder(dimensionNames, true); // Create stats for the following dimension sets - List> populatedStats = new ArrayList<>(); + List> statsToPopulate = 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)); + statsToPopulate.add(List.of("A" + indexA, "B" + indexB)); } } - for (List dims : populatedStats) { - cacheStatsHolder.incrementHits(dims); - } + setupRemovalTest(cacheStatsHolder, statsToPopulate, true); // 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. @@ -200,13 +192,33 @@ public void testConcurrentRemoval() throws Exception { 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()); + 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()); } } + static void setupRemovalTest( + TieredSpilloverCacheStatsHolder cacheStatsHolder, + List> statsToPopulate, + boolean diskTierEnabled + ) { + List tiers = diskTierEnabled + ? List.of(TIER_DIMENSION_VALUE_ON_HEAP, TIER_DIMENSION_VALUE_DISK) + : List.of(TIER_DIMENSION_VALUE_ON_HEAP); + for (List dims : statsToPopulate) { + // Increment hits, misses, and evictions for set of dimensions, for both heap and disk + for (String tier : tiers) { + List dimsWithDimension = cacheStatsHolder.getDimensionsWithTierValue(dims, tier); + cacheStatsHolder.incrementHits(dimsWithDimension); + cacheStatsHolder.incrementMisses(dimsWithDimension); + boolean includeInTotal = tier.equals(TIER_DIMENSION_VALUE_DISK) || !diskTierEnabled; + cacheStatsHolder.incrementEvictions(dimsWithDimension, includeInTotal); + } + } + } + /** * Returns the node found by following these dimension values down from the root node. * Returns null if no such node exists. @@ -226,16 +238,24 @@ static Map, CacheStats> populateStats( TieredSpilloverCacheStatsHolder cacheStatsHolder, Map> usedDimensionValues, int numDistinctValuePairs, - int numRepetitionsPerValue + int numRepetitionsPerValue, + boolean diskTierEnabled ) throws InterruptedException { - return populateStats(List.of(cacheStatsHolder), usedDimensionValues, numDistinctValuePairs, numRepetitionsPerValue); + return populateStats( + List.of(cacheStatsHolder), + usedDimensionValues, + numDistinctValuePairs, + numRepetitionsPerValue, + diskTierEnabled + ); } static Map, CacheStats> populateStats( List cacheStatsHolders, Map> usedDimensionValues, int numDistinctValuePairs, - int numRepetitionsPerValue + int numRepetitionsPerValue, + boolean diskTierEnabled ) throws InterruptedException { for (TieredSpilloverCacheStatsHolder statsHolder : cacheStatsHolders) { assertEquals(cacheStatsHolders.get(0).getDimensionNames(), statsHolder.getDimensionNames()); @@ -266,7 +286,7 @@ static Map, CacheStats> populateStats( expected.get(dimensions).evictions.inc(statsToInc.getEvictions()); expected.get(dimensions).sizeInBytes.inc(statsToInc.getSizeInBytes()); expected.get(dimensions).items.inc(statsToInc.getItems()); - populateStatsHolderFromStatsValueMap(cacheStatsHolder, Map.of(dimensions, statsToInc)); + populateStatsHolderFromStatsValueMap(cacheStatsHolder, Map.of(dimensions, statsToInc), diskTierEnabled); } } countDownLatch.countDown(); @@ -296,7 +316,11 @@ private static List getRandomDimList( return result; } - static Map> getUsedDimensionValues(TieredSpilloverCacheStatsHolder cacheStatsHolder, int numValuesPerDim) { + static Map> getUsedDimensionValues( + TieredSpilloverCacheStatsHolder cacheStatsHolder, + int numValuesPerDim, + boolean diskTierEnabled + ) { Map> usedDimensionValues = new HashMap<>(); for (int i = 0; i < cacheStatsHolder.getDimensionNames().size() - 1; i++) { // Have to handle final tier dimension separately List values = new ArrayList<>(); @@ -305,27 +329,19 @@ static Map> getUsedDimensionValues(TieredSpilloverCacheStat } usedDimensionValues.put(cacheStatsHolder.getDimensionNames().get(i), values); } - List tierValues = List.of(TIER_DIMENSION_VALUE_ON_HEAP, TIER_DIMENSION_VALUE_DISK); - usedDimensionValues.put(TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME, tierValues); - return usedDimensionValues; - } - - private void assertSumOfChildrenStats(DefaultCacheStatsHolder.Node current) { - if (!current.children.isEmpty()) { - CacheStats expectedTotal = new CacheStats(); - for (DefaultCacheStatsHolder.Node child : current.children.values()) { - expectedTotal.add(child.getImmutableStats()); - } - assertEquals(expectedTotal.immutableSnapshot(), current.getImmutableStats()); - for (DefaultCacheStatsHolder.Node child : current.children.values()) { - assertSumOfChildrenStats(child); - } + if (diskTierEnabled) { + List tierValues = List.of(TIER_DIMENSION_VALUE_ON_HEAP, TIER_DIMENSION_VALUE_DISK); + usedDimensionValues.put(TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME, tierValues); + } else { + usedDimensionValues.put(TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME, List.of(TIER_DIMENSION_VALUE_ON_HEAP)); } + return usedDimensionValues; } public static void populateStatsHolderFromStatsValueMap( TieredSpilloverCacheStatsHolder cacheStatsHolder, - Map, CacheStats> statsMap + Map, CacheStats> statsMap, + boolean diskTierEnabled ) { for (Map.Entry, CacheStats> entry : statsMap.entrySet()) { CacheStats stats = entry.getValue(); @@ -337,10 +353,9 @@ public static void populateStatsHolderFromStatsValueMap( cacheStatsHolder.incrementMisses(dims); } for (int i = 0; i < stats.getEvictions(); i++) { - // TODO: for now include all disk evictions in total - boolean includeInTotal = dims.get(dims.size()-1).equals(TIER_DIMENSION_VALUE_DISK); - //System.out.println("Include in total = " + includeInTotal); - cacheStatsHolder.incrementEvictions(dims, true); + // For these tests, don't include heap evictions in total (as if there were no policies + disk tier is active) + boolean includeInTotal = dims.get(dims.size() - 1).equals(TIER_DIMENSION_VALUE_DISK) || !diskTierEnabled; + cacheStatsHolder.incrementEvictions(dims, includeInTotal); } cacheStatsHolder.incrementSizeInBytes(dims, stats.getSizeInBytes()); for (int i = 0; i < stats.getItems(); i++) { diff --git a/server/src/main/java/org/opensearch/common/cache/stats/CacheStats.java b/server/src/main/java/org/opensearch/common/cache/stats/CacheStats.java index 5312961bb7591..93fa1ff7fcddf 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/CacheStats.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/CacheStats.java @@ -16,11 +16,11 @@ * A mutable class containing the 5 live metrics tracked by a StatsHolder object. */ public class CacheStats { - public CounterMetric hits; - public CounterMetric misses; - public CounterMetric evictions; - public CounterMetric sizeInBytes; - public CounterMetric items; + CounterMetric hits; + CounterMetric misses; + CounterMetric evictions; + CounterMetric sizeInBytes; + CounterMetric items; public CacheStats(long hits, long misses, long evictions, long sizeInBytes, long items) { this.hits = new CounterMetric(); 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 6f0d9faa0158f..d62736f516e24 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 @@ -209,14 +209,14 @@ private ImmutableCacheStats removeDimensionsHelper(List dimensionValues, } // pkg-private for testing - 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. public final Map children; @@ -241,7 +241,7 @@ public String getDimensionValue() { return dimensionValue; } - public Map getChildren() { + protected Map getChildren() { // We can safely iterate over ConcurrentHashMap without worrying about thread issues. return children; } @@ -280,7 +280,7 @@ long getEntries() { return this.stats.getItems(); } - public ImmutableCacheStats getImmutableStats() { + ImmutableCacheStats getImmutableStats() { return this.stats.immutableSnapshot(); } From 50707b0bb7890614f93f4a5e34774a24ea7a997b Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 12:08:01 -0800 Subject: [PATCH 04/12] Cleanup Signed-off-by: Peter Alfonsi --- .../common/tier/TieredSpilloverCache.java | 2 +- .../TieredSpilloverCacheStatsHolder.java | 32 ++++++------------- .../TieredSpilloverCacheStatsHolderTests.java | 8 ++--- .../cache/stats/DefaultCacheStatsHolder.java | 10 ++++-- 4 files changed, 21 insertions(+), 31 deletions(-) diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java index 6cecdcab8fb33..a6ea36dc501e7 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java @@ -377,7 +377,7 @@ public void invalidate(ICacheKey key) { for (Map.Entry, TierInfo> cacheEntry : caches.entrySet()) { if (key.getDropStatsForDimensions()) { List dimensionValues = statsHolder.getDimensionsWithTierValue(key.dimensions, cacheEntry.getValue().tierName); - statsHolder.removeDimensions(dimensionValues); + statsHolder.removeDimensions(dimensionValues); // TODO: fix!! } if (key.key != null) { try (ReleasableLock ignore = writeLock.acquire()) { diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java index a9151c9a7a3ee..76ee9b82e0834 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java @@ -43,6 +43,8 @@ public class TieredSpilloverCacheStatsHolder extends DefaultCacheStatsHolder { /** Dimension value for on-disk cache, like EhcacheDiskCache. */ public static final String TIER_DIMENSION_VALUE_DISK = "disk"; + static final List TIER_VALUES = List.of(TIER_DIMENSION_VALUE_ON_HEAP, TIER_DIMENSION_VALUE_DISK); + /** * Constructor for the stats holder. * @param originalDimensionNames the original dimension names, not including TIER_DIMENSION_NAME @@ -181,28 +183,14 @@ public void removeDimensions(List dimensionValues) { } } - private ImmutableCacheStats removeDimensionsHelper(List dimensionValues, Node node, int depth) { - if (depth == dimensionValues.size()) { // dimensionValues passed in doesn't include the tier dimension - // Manually delete this node's children (which represent individual tiers) TODO: is this needed or does GC get it anyway? - for (Node child : node.getChildren().values()) { - // TODO: iteration issues? - node.children.remove(child.getDimensionValue()); - } - // Pass up a snapshot of the original stats to avoid issues when the original is decremented by other fn invocations - return node.getImmutableStats(); - } - Node child = node.getChild(dimensionValues.get(depth)); - if (child == null) { - return null; - } - ImmutableCacheStats statsToDecrement = removeDimensionsHelper(dimensionValues, child, depth + 1); - if (statsToDecrement != null) { - // The removal took place, decrement values and remove this node from its parent if it's now empty - node.decrementBySnapshot(statsToDecrement); - if (child.getChildren().isEmpty()) { - node.children.remove(child.getDimensionValue()); - } + @Override + protected ImmutableCacheStats removeDimensionsBaseCase(Node node) { + // The base case will be the node whose children represent individual tiers. + // Manually delete this node's children + for (String tierValue : TIER_VALUES) { + node.children.remove(tierValue); } - return statsToDecrement; + // Pass up a snapshot of the original stats to avoid issues when the original is decremented by other fn invocations + return node.getImmutableStats(); } } diff --git a/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java b/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java index c7fe173113e5f..ab8e7e57aa2c1 100644 --- a/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java @@ -23,6 +23,7 @@ import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_VALUES; public class TieredSpilloverCacheStatsHolderTests extends OpenSearchTestCase { // These are modified from DefaultCacheStatsHolderTests.java to account for the tiers. Because we can't add a dependency on server.test, @@ -204,9 +205,7 @@ static void setupRemovalTest( List> statsToPopulate, boolean diskTierEnabled ) { - List tiers = diskTierEnabled - ? List.of(TIER_DIMENSION_VALUE_ON_HEAP, TIER_DIMENSION_VALUE_DISK) - : List.of(TIER_DIMENSION_VALUE_ON_HEAP); + List tiers = diskTierEnabled ? TIER_VALUES : List.of(TIER_DIMENSION_VALUE_ON_HEAP); for (List dims : statsToPopulate) { // Increment hits, misses, and evictions for set of dimensions, for both heap and disk for (String tier : tiers) { @@ -330,8 +329,7 @@ static Map> getUsedDimensionValues( usedDimensionValues.put(cacheStatsHolder.getDimensionNames().get(i), values); } if (diskTierEnabled) { - List tierValues = List.of(TIER_DIMENSION_VALUE_ON_HEAP, TIER_DIMENSION_VALUE_DISK); - usedDimensionValues.put(TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME, tierValues); + usedDimensionValues.put(TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME, TIER_VALUES); } else { usedDimensionValues.put(TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME, List.of(TIER_DIMENSION_VALUE_ON_HEAP)); } 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 d62736f516e24..0cec60c479b4e 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 @@ -188,10 +188,10 @@ public void removeDimensions(List dimensionValues) { } // Returns a CacheStatsCounterSnapshot object for the stats to decrement if the removal happened, null otherwise. - private ImmutableCacheStats removeDimensionsHelper(List dimensionValues, Node node, int depth) { + protected ImmutableCacheStats removeDimensionsHelper(List dimensionValues, Node node, int depth) { if (depth == dimensionValues.size()) { // Pass up a snapshot of the original stats to avoid issues when the original is decremented by other fn invocations - return node.getImmutableStats(); + return removeDimensionsBaseCase(node); } Node child = node.getChild(dimensionValues.get(depth)); if (child == null) { @@ -208,6 +208,10 @@ private ImmutableCacheStats removeDimensionsHelper(List dimensionValues, return statsToDecrement; } + protected ImmutableCacheStats removeDimensionsBaseCase(Node node) { + return node.getImmutableStats(); + } + // pkg-private for testing Node getStatsRoot() { return statsRoot; @@ -219,7 +223,7 @@ Node getStatsRoot() { protected static class Node { private final String dimensionValue; // Map from dimensionValue to the DimensionNode for that dimension value. - public final Map children; + final Map children; // The stats for this node. If a leaf node, corresponds to the stats for this combination of dimensions; if not, // contains the sum of its children's stats. private CacheStats stats; From afd22e8e5c4c91167654e754be91244cf6f607a5 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 12:32:10 -0800 Subject: [PATCH 05/12] Integrate fix with TSC Signed-off-by: Peter Alfonsi --- .../common/tier/TieredSpilloverCache.java | 16 +++--- .../tier/TieredSpilloverCacheTests.java | 54 +++++++++++++++++++ 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java index a6ea36dc501e7..be777d901d719 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java @@ -374,14 +374,14 @@ private V compute(ICacheKey key, LoadAwareCacheLoader, V> loader @Override public void invalidate(ICacheKey key) { - for (Map.Entry, TierInfo> cacheEntry : caches.entrySet()) { - if (key.getDropStatsForDimensions()) { - List dimensionValues = statsHolder.getDimensionsWithTierValue(key.dimensions, cacheEntry.getValue().tierName); - statsHolder.removeDimensions(dimensionValues); // TODO: fix!! - } - if (key.key != null) { - try (ReleasableLock ignore = writeLock.acquire()) { - cacheEntry.getKey().invalidate(key); + if (key.getDropStatsForDimensions()) { + statsHolder.removeDimensions(key.dimensions); + } else { + for (Map.Entry, TierInfo> cacheEntry : caches.entrySet()) { + if (key.key != null) { + try (ReleasableLock ignore = writeLock.acquire()) { + cacheEntry.getKey().invalidate(key); + } } } } diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java index 051aa0354babe..33d8e41718790 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java @@ -2112,6 +2112,60 @@ public void testTieredCacheDefaultSegmentCount() { assertTrue(VALID_SEGMENT_COUNT_VALUES.contains(tieredSpilloverCache.getNumberOfSegments())); } + public void testDropStatsForDimensions() throws Exception { + int onHeapCacheSize = randomIntBetween(300, 600); + int diskCacheSize = randomIntBetween(700, 1200); + int numberOfSegments = getNumberOfSegments(); + int keyValueSize = 50; + MockCacheRemovalListener removalListener = new MockCacheRemovalListener<>(); + TieredSpilloverCache tieredSpilloverCache = initializeTieredSpilloverCache( + keyValueSize, + diskCacheSize, + removalListener, + Settings.builder() + .put( + TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE.getConcreteSettingForNamespace( + CacheType.INDICES_REQUEST_CACHE.getSettingPrefix() + ).getKey(), + onHeapCacheSize * keyValueSize + "b" + ) + .build(), + 0, + numberOfSegments + ); + + List> usedKeys = new ArrayList<>(); + // Fill the cache, getting some entries + evictions for both tiers + int minMisses = (diskCacheSize + onHeapCacheSize) / keyValueSize + 10; + int numMisses = onHeapCacheSize + diskCacheSize + randomIntBetween(minMisses, minMisses + 50); + for (int iter = 0; iter < numMisses; iter++) { + ICacheKey key = getICacheKey(UUID.randomUUID().toString()); + usedKeys.add(key); + LoadAwareCacheLoader, String> tieredCacheLoader = getLoadAwareCacheLoader(); + tieredSpilloverCache.computeIfAbsent(key, tieredCacheLoader); + } + // Also do some random hits + Random rand = Randomness.get(); + int approxNumHits = 30; + for (int i = 0; i < approxNumHits; i++) { + LoadAwareCacheLoader, String> tieredCacheLoader = getLoadAwareCacheLoader(); + ICacheKey key = usedKeys.get(rand.nextInt(usedKeys.size())); + tieredSpilloverCache.computeIfAbsent(key, tieredCacheLoader); + } + + ImmutableCacheStats totalStats = tieredSpilloverCache.stats().getTotalStats(); + assertTrue(totalStats.getHits() > 0); + assertTrue(totalStats.getMisses() > 0); + assertTrue(totalStats.getEvictions() > 0); + + // Since all the keys have the same dimension values, except tiers, we only need to remove that one, and we expect all stats values + // should be 0 after that. + ICacheKey dropDimensionsKey = new ICacheKey<>(null, getMockDimensions()); + dropDimensionsKey.setDropStatsForDimensions(true); + tieredSpilloverCache.invalidate(dropDimensionsKey); + assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), tieredSpilloverCache.stats().getTotalStats()); + } + private List getMockDimensions() { List dims = new ArrayList<>(); for (String dimensionName : dimensionNames) { From 902041a92ea8232354dd336df181e10989226a20 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 12:54:42 -0800 Subject: [PATCH 06/12] Add IT Signed-off-by: Peter Alfonsi --- .../tier/TieredSpilloverCacheStatsIT.java | 51 +++++++++++++++++++ .../TieredSpilloverCacheStatsHolderTests.java | 1 - 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java index 72dfd387f7a17..e7f0100f9ea12 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java @@ -10,6 +10,7 @@ import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; +import org.opensearch.action.admin.indices.delete.DeleteIndexRequest; import org.opensearch.action.admin.indices.forcemerge.ForceMergeResponse; import org.opensearch.action.admin.indices.stats.CommonStatsFlags; import org.opensearch.action.search.SearchResponse; @@ -40,6 +41,7 @@ import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.indices.IndicesService.INDICES_CACHE_CLEAN_INTERVAL_SETTING; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse; @@ -417,6 +419,55 @@ public void testStatsWithMultipleSegments() throws Exception { assertTrue(diskCacheStat.getEvictions() == 0); } + public void testClosingShard() throws Exception { + // Closing the shard should totally remove the stats associated with that shard. + internalCluster().startNodes( + 1, + Settings.builder() + .put(defaultSettings(HEAP_CACHE_SIZE_STRING, getNumberOfSegments())) + .put( + TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP.get(CacheType.INDICES_REQUEST_CACHE).getKey(), + new TimeValue(0, TimeUnit.SECONDS) + ) + .put(INDICES_CACHE_CLEAN_INTERVAL_SETTING.getKey(), new TimeValue(1)) + .build() + ); + String index = "index"; + Client client = client(); + startIndex(client, index); + + // First search one time to see how big a single value will be + searchIndex(client, index, 0); + // get total stats + long singleSearchSize = getTotalStats(client).getSizeInBytes(); + // Select numbers so we get some values on both heap and disk + int itemsOnHeap = HEAP_CACHE_SIZE / (int) singleSearchSize; + int itemsOnDisk = 1 + randomInt(30); // The first one we search (to get the size) always goes to disk + int expectedEntries = itemsOnHeap + itemsOnDisk; + + for (int i = 1; i < expectedEntries; i++) { + // Cause misses + searchIndex(client, index, i); + } + int expectedMisses = itemsOnHeap + itemsOnDisk; + + // Cause some hits + int expectedHits = randomIntBetween(itemsOnHeap, expectedEntries); // Select it so some hits come from both tiers + for (int i = 0; i < expectedHits; i++) { + searchIndex(client, index, i); + } + + // Check the new stats API values are as expected + assertEquals( + new ImmutableCacheStats(expectedHits, expectedMisses, 0, expectedEntries * singleSearchSize, expectedEntries), + getTotalStats(client) + ); + + // Closing the index should close the shard + assertAcked(client().admin().indices().delete(new DeleteIndexRequest("index")).get()); + assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), getTotalStats(client)); + } + private void startIndex(Client client, String indexName) throws InterruptedException { assertAcked( client.admin() diff --git a/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java b/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java index ab8e7e57aa2c1..79b86c5f6d080 100644 --- a/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java @@ -351,7 +351,6 @@ public static void populateStatsHolderFromStatsValueMap( cacheStatsHolder.incrementMisses(dims); } for (int i = 0; i < stats.getEvictions(); i++) { - // For these tests, don't include heap evictions in total (as if there were no policies + disk tier is active) boolean includeInTotal = dims.get(dims.size() - 1).equals(TIER_DIMENSION_VALUE_DISK) || !diskTierEnabled; cacheStatsHolder.incrementEvictions(dims, includeInTotal); } From 3b15a7a4795b7638deb2998cd3d060d5a87e26a1 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 13:29:11 -0800 Subject: [PATCH 07/12] Refactor cache package names in TSC module to match with server Signed-off-by: Peter Alfonsi --- .../cache}/tier/TieredSpilloverCacheBaseIT.java | 2 +- .../cache}/tier/TieredSpilloverCacheIT.java | 4 ++-- .../cache}/tier/TieredSpilloverCacheStatsIT.java | 2 +- .../cache}/policy/TookTimePolicy.java | 5 ++--- .../common => common/cache}/policy/package-info.java | 2 +- .../cache/stats/TieredSpilloverCacheStatsHolder.java | 2 +- .../cache}/tier/TieredSpilloverCache.java | 12 ++++++------ .../cache}/tier/TieredSpilloverCachePlugin.java | 6 +++--- .../cache}/tier/TieredSpilloverCacheSettings.java | 2 +- .../common => common/cache}/tier/package-info.java | 2 +- .../cache}/policy/TookTimePolicyTests.java | 5 ++--- .../common => common/cache}/tier/MockDiskCache.java | 2 +- .../cache}/tier/TieredSpilloverCachePluginTests.java | 2 +- .../cache}/tier/TieredSpilloverCacheTests.java | 12 ++++++------ 14 files changed, 29 insertions(+), 31 deletions(-) rename modules/cache-common/src/internalClusterTest/java/org/opensearch/{cache/common => common/cache}/tier/TieredSpilloverCacheBaseIT.java (98%) rename modules/cache-common/src/internalClusterTest/java/org/opensearch/{cache/common => common/cache}/tier/TieredSpilloverCacheIT.java (99%) rename modules/cache-common/src/internalClusterTest/java/org/opensearch/{cache/common => common/cache}/tier/TieredSpilloverCacheStatsIT.java (99%) rename modules/cache-common/src/main/java/org/opensearch/{cache/common => common/cache}/policy/TookTimePolicy.java (94%) rename modules/cache-common/src/main/java/org/opensearch/{cache/common => common/cache}/policy/package-info.java (85%) rename modules/cache-common/src/main/java/org/opensearch/{cache/common => common/cache}/tier/TieredSpilloverCache.java (99%) rename modules/cache-common/src/main/java/org/opensearch/{cache/common => common/cache}/tier/TieredSpilloverCachePlugin.java (94%) rename modules/cache-common/src/main/java/org/opensearch/{cache/common => common/cache}/tier/TieredSpilloverCacheSettings.java (99%) rename modules/cache-common/src/main/java/org/opensearch/{cache/common => common/cache}/tier/package-info.java (85%) rename modules/cache-common/src/test/java/org/opensearch/{cache/common => common/cache}/policy/TookTimePolicyTests.java (96%) rename modules/cache-common/src/test/java/org/opensearch/{cache/common => common/cache}/tier/MockDiskCache.java (99%) rename modules/cache-common/src/test/java/org/opensearch/{cache/common => common/cache}/tier/TieredSpilloverCachePluginTests.java (96%) rename modules/cache-common/src/test/java/org/opensearch/{cache/common => common/cache}/tier/TieredSpilloverCacheTests.java (99%) diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheBaseIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheBaseIT.java similarity index 98% rename from modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheBaseIT.java rename to modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheBaseIT.java index 01371ca8eeefb..5e1d474d5c823 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheBaseIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheBaseIT.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.settings.CacheSettings; diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheIT.java similarity index 99% rename from modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheIT.java rename to modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheIT.java index d58e36c036510..3d5e3d0c9aab8 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheIT.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.opensearch.action.admin.cluster.node.info.NodeInfo; import org.opensearch.action.admin.cluster.node.info.NodesInfoRequest; @@ -76,7 +76,7 @@ public void testPluginsAreInstalled() { .collect(Collectors.toList()); Assert.assertTrue( pluginInfos.stream() - .anyMatch(pluginInfo -> pluginInfo.getName().equals("org.opensearch.cache.common.tier.TieredSpilloverCachePlugin")) + .anyMatch(pluginInfo -> pluginInfo.getName().equals("org.opensearch.common.cache.tier.TieredSpilloverCachePlugin")) ); } diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheStatsIT.java similarity index 99% rename from modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java rename to modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheStatsIT.java index e7f0100f9ea12..83515553d5070 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheStatsIT.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/policy/TookTimePolicy.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/policy/TookTimePolicy.java similarity index 94% rename from modules/cache-common/src/main/java/org/opensearch/cache/common/policy/TookTimePolicy.java rename to modules/cache-common/src/main/java/org/opensearch/common/cache/policy/TookTimePolicy.java index 4bc26803acf4c..2617ac398d204 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/policy/TookTimePolicy.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/policy/TookTimePolicy.java @@ -11,17 +11,16 @@ * GitHub history for details. */ -package org.opensearch.cache.common.policy; +package org.opensearch.common.cache.policy; import org.opensearch.common.cache.CacheType; -import org.opensearch.common.cache.policy.CachedQueryResult; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.unit.TimeValue; import java.util.function.Function; import java.util.function.Predicate; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; /** * A cache tier policy which accepts queries whose took time is greater than some threshold. diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/policy/package-info.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/policy/package-info.java similarity index 85% rename from modules/cache-common/src/main/java/org/opensearch/cache/common/policy/package-info.java rename to modules/cache-common/src/main/java/org/opensearch/common/cache/policy/package-info.java index 45cfb00662c98..8a1f59a96cd3e 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/policy/package-info.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/policy/package-info.java @@ -7,4 +7,4 @@ */ /** A package for policies controlling what can enter caches. */ -package org.opensearch.cache.common.policy; +package org.opensearch.common.cache.policy; diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java index 76ee9b82e0834..9df34188de2f8 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java @@ -8,7 +8,7 @@ package org.opensearch.common.cache.stats; -import org.opensearch.cache.common.tier.TieredSpilloverCache; +import org.opensearch.common.cache.tier.TieredSpilloverCache; import java.util.ArrayList; import java.util.List; diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCache.java similarity index 99% rename from modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java rename to modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCache.java index be777d901d719..c970a7bb6355b 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCache.java @@ -6,11 +6,10 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.opensearch.cache.common.policy.TookTimePolicy; import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.ICache; @@ -20,6 +19,7 @@ import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.policy.CachedQueryResult; +import org.opensearch.common.cache.policy.TookTimePolicy; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder; import org.opensearch.common.cache.store.config.CacheConfig; @@ -50,14 +50,14 @@ import java.util.function.Predicate; import java.util.function.ToLongBiFunction; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; /** * This cache spillover the evicted items from heap tier to disk tier. All the new items are first cached on heap diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCachePlugin.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCachePlugin.java similarity index 94% rename from modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCachePlugin.java rename to modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCachePlugin.java index bf522b42b70ca..6929f3d24988c 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCachePlugin.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCachePlugin.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.ICache; @@ -20,8 +20,8 @@ import java.util.List; import java.util.Map; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; /** * Plugin for TieredSpilloverCache. diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCacheSettings.java similarity index 99% rename from modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java rename to modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCacheSettings.java index 122d00af3bd1e..72e133f0a53a9 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCacheSettings.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.opensearch.common.cache.CacheType; import org.opensearch.common.settings.Setting; diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/package-info.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/package-info.java similarity index 85% rename from modules/cache-common/src/main/java/org/opensearch/cache/common/tier/package-info.java rename to modules/cache-common/src/main/java/org/opensearch/common/cache/tier/package-info.java index fa2de3c14b5dc..e8c457370695a 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/package-info.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/package-info.java @@ -7,4 +7,4 @@ */ /** Package related to cache tiers **/ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/policy/TookTimePolicyTests.java b/modules/cache-common/src/test/java/org/opensearch/common/cache/policy/TookTimePolicyTests.java similarity index 96% rename from modules/cache-common/src/test/java/org/opensearch/cache/common/policy/TookTimePolicyTests.java rename to modules/cache-common/src/test/java/org/opensearch/common/cache/policy/TookTimePolicyTests.java index 000067280e50d..4d05c1d13fb59 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/policy/TookTimePolicyTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/common/cache/policy/TookTimePolicyTests.java @@ -6,14 +6,13 @@ * compatible open source license. */ -package org.opensearch.cache.common.policy; +package org.opensearch.common.cache.policy; import org.apache.lucene.search.ScoreDoc; import org.apache.lucene.search.TopDocs; import org.apache.lucene.search.TotalHits; import org.opensearch.common.Randomness; import org.opensearch.common.cache.CacheType; -import org.opensearch.common.cache.policy.CachedQueryResult; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.lucene.search.TopDocsAndMaxScore; import org.opensearch.common.settings.ClusterSettings; @@ -31,7 +30,7 @@ import java.util.Random; import java.util.function.Function; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; public class TookTimePolicyTests extends OpenSearchTestCase { private final Function transformationFunction = (data) -> { diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java b/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/MockDiskCache.java similarity index 99% rename from modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java rename to modules/cache-common/src/test/java/org/opensearch/common/cache/tier/MockDiskCache.java index fcddd489a27aa..da0885c80e20f 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java +++ b/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/MockDiskCache.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.ICache; diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCachePluginTests.java b/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCachePluginTests.java similarity index 96% rename from modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCachePluginTests.java rename to modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCachePluginTests.java index 4a96ffe2069ec..42d397875503e 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCachePluginTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCachePluginTests.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.opensearch.common.cache.ICache; import org.opensearch.common.settings.Settings; diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCacheTests.java similarity index 99% rename from modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java rename to modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCacheTests.java index 33d8e41718790..ca8a0f3a4300c 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCacheTests.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.cache.common.tier; +package org.opensearch.common.cache.tier; import org.opensearch.OpenSearchException; import org.opensearch.common.Randomness; @@ -56,17 +56,17 @@ import java.util.function.Function; import java.util.function.Predicate; -import static org.opensearch.cache.common.tier.TieredSpilloverCache.ZERO_SEGMENT_COUNT_EXCEPTION_MESSAGE; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; -import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.common.cache.store.settings.OpenSearchOnHeapCacheSettings.MAXIMUM_SIZE_IN_BYTES_KEY; +import static org.opensearch.common.cache.tier.TieredSpilloverCache.ZERO_SEGMENT_COUNT_EXCEPTION_MESSAGE; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; +import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; From c2a05d4ebfcd7471523679047fedb9dc1ed28e23 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 14:23:58 -0800 Subject: [PATCH 08/12] changelog Signed-off-by: Peter Alfonsi --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bf90a75d0148..c037708e90091 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix warnings from SLF4J on startup when repository-s3 is installed ([#16194](https://github.com/opensearch-project/OpenSearch/pull/16194)) - Fix protobuf-java leak through client library dependencies ([#16254](https://github.com/opensearch-project/OpenSearch/pull/16254)) - Fix multi-search with template doesn't return status code ([#16265](https://github.com/opensearch-project/OpenSearch/pull/16265)) +- Fix bug in new cache stats API when closing shards while using TieredSpilloverCache ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560)) ### Security From dc515d9cb738436635cf9f1e88bed7b1a38f7068 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 16:33:29 -0800 Subject: [PATCH 09/12] Revert "Refactor cache package names in TSC module to match with server" This reverts commit 3b15a7a4795b7638deb2998cd3d060d5a87e26a1. Signed-off-by: Peter Alfonsi --- .../common}/tier/TieredSpilloverCacheBaseIT.java | 2 +- .../common}/tier/TieredSpilloverCacheIT.java | 4 ++-- .../common}/tier/TieredSpilloverCacheStatsIT.java | 2 +- .../common}/policy/TookTimePolicy.java | 5 +++-- .../cache => cache/common}/policy/package-info.java | 2 +- .../common}/tier/TieredSpilloverCache.java | 12 ++++++------ .../common}/tier/TieredSpilloverCachePlugin.java | 6 +++--- .../common}/tier/TieredSpilloverCacheSettings.java | 2 +- .../cache => cache/common}/tier/package-info.java | 2 +- .../cache/stats/TieredSpilloverCacheStatsHolder.java | 2 +- .../common}/policy/TookTimePolicyTests.java | 5 +++-- .../cache => cache/common}/tier/MockDiskCache.java | 2 +- .../tier/TieredSpilloverCachePluginTests.java | 2 +- .../common}/tier/TieredSpilloverCacheTests.java | 12 ++++++------ 14 files changed, 31 insertions(+), 29 deletions(-) rename modules/cache-common/src/internalClusterTest/java/org/opensearch/{common/cache => cache/common}/tier/TieredSpilloverCacheBaseIT.java (98%) rename modules/cache-common/src/internalClusterTest/java/org/opensearch/{common/cache => cache/common}/tier/TieredSpilloverCacheIT.java (99%) rename modules/cache-common/src/internalClusterTest/java/org/opensearch/{common/cache => cache/common}/tier/TieredSpilloverCacheStatsIT.java (99%) rename modules/cache-common/src/main/java/org/opensearch/{common/cache => cache/common}/policy/TookTimePolicy.java (94%) rename modules/cache-common/src/main/java/org/opensearch/{common/cache => cache/common}/policy/package-info.java (85%) rename modules/cache-common/src/main/java/org/opensearch/{common/cache => cache/common}/tier/TieredSpilloverCache.java (99%) rename modules/cache-common/src/main/java/org/opensearch/{common/cache => cache/common}/tier/TieredSpilloverCachePlugin.java (94%) rename modules/cache-common/src/main/java/org/opensearch/{common/cache => cache/common}/tier/TieredSpilloverCacheSettings.java (99%) rename modules/cache-common/src/main/java/org/opensearch/{common/cache => cache/common}/tier/package-info.java (85%) rename modules/cache-common/src/test/java/org/opensearch/{common/cache => cache/common}/policy/TookTimePolicyTests.java (96%) rename modules/cache-common/src/test/java/org/opensearch/{common/cache => cache/common}/tier/MockDiskCache.java (99%) rename modules/cache-common/src/test/java/org/opensearch/{common/cache => cache/common}/tier/TieredSpilloverCachePluginTests.java (96%) rename modules/cache-common/src/test/java/org/opensearch/{common/cache => cache/common}/tier/TieredSpilloverCacheTests.java (99%) diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheBaseIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheBaseIT.java similarity index 98% rename from modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheBaseIT.java rename to modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheBaseIT.java index 5e1d474d5c823..01371ca8eeefb 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheBaseIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheBaseIT.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.settings.CacheSettings; diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheIT.java similarity index 99% rename from modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheIT.java rename to modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheIT.java index 3d5e3d0c9aab8..d58e36c036510 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheIT.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.opensearch.action.admin.cluster.node.info.NodeInfo; import org.opensearch.action.admin.cluster.node.info.NodesInfoRequest; @@ -76,7 +76,7 @@ public void testPluginsAreInstalled() { .collect(Collectors.toList()); Assert.assertTrue( pluginInfos.stream() - .anyMatch(pluginInfo -> pluginInfo.getName().equals("org.opensearch.common.cache.tier.TieredSpilloverCachePlugin")) + .anyMatch(pluginInfo -> pluginInfo.getName().equals("org.opensearch.cache.common.tier.TieredSpilloverCachePlugin")) ); } diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheStatsIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java similarity index 99% rename from modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheStatsIT.java rename to modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java index 83515553d5070..e7f0100f9ea12 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheStatsIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/policy/TookTimePolicy.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/policy/TookTimePolicy.java similarity index 94% rename from modules/cache-common/src/main/java/org/opensearch/common/cache/policy/TookTimePolicy.java rename to modules/cache-common/src/main/java/org/opensearch/cache/common/policy/TookTimePolicy.java index 2617ac398d204..4bc26803acf4c 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/policy/TookTimePolicy.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/policy/TookTimePolicy.java @@ -11,16 +11,17 @@ * GitHub history for details. */ -package org.opensearch.common.cache.policy; +package org.opensearch.cache.common.policy; import org.opensearch.common.cache.CacheType; +import org.opensearch.common.cache.policy.CachedQueryResult; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.unit.TimeValue; import java.util.function.Function; import java.util.function.Predicate; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; /** * A cache tier policy which accepts queries whose took time is greater than some threshold. diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/policy/package-info.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/policy/package-info.java similarity index 85% rename from modules/cache-common/src/main/java/org/opensearch/common/cache/policy/package-info.java rename to modules/cache-common/src/main/java/org/opensearch/cache/common/policy/package-info.java index 8a1f59a96cd3e..45cfb00662c98 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/policy/package-info.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/policy/package-info.java @@ -7,4 +7,4 @@ */ /** A package for policies controlling what can enter caches. */ -package org.opensearch.common.cache.policy; +package org.opensearch.cache.common.policy; diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java similarity index 99% rename from modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCache.java rename to modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java index c970a7bb6355b..be777d901d719 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java @@ -6,10 +6,11 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.opensearch.cache.common.policy.TookTimePolicy; import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.ICache; @@ -19,7 +20,6 @@ import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.policy.CachedQueryResult; -import org.opensearch.common.cache.policy.TookTimePolicy; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder; import org.opensearch.common.cache.store.config.CacheConfig; @@ -50,14 +50,14 @@ import java.util.function.Predicate; import java.util.function.ToLongBiFunction; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; /** * This cache spillover the evicted items from heap tier to disk tier. All the new items are first cached on heap diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCachePlugin.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCachePlugin.java similarity index 94% rename from modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCachePlugin.java rename to modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCachePlugin.java index 6929f3d24988c..bf522b42b70ca 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCachePlugin.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCachePlugin.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.ICache; @@ -20,8 +20,8 @@ import java.util.List; import java.util.Map; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; /** * Plugin for TieredSpilloverCache. diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCacheSettings.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java similarity index 99% rename from modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCacheSettings.java rename to modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java index 72e133f0a53a9..122d00af3bd1e 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/TieredSpilloverCacheSettings.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.opensearch.common.cache.CacheType; import org.opensearch.common.settings.Setting; diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/package-info.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/package-info.java similarity index 85% rename from modules/cache-common/src/main/java/org/opensearch/common/cache/tier/package-info.java rename to modules/cache-common/src/main/java/org/opensearch/cache/common/tier/package-info.java index e8c457370695a..fa2de3c14b5dc 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/tier/package-info.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/package-info.java @@ -7,4 +7,4 @@ */ /** Package related to cache tiers **/ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java index 9df34188de2f8..76ee9b82e0834 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java +++ b/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java @@ -8,7 +8,7 @@ package org.opensearch.common.cache.stats; -import org.opensearch.common.cache.tier.TieredSpilloverCache; +import org.opensearch.cache.common.tier.TieredSpilloverCache; import java.util.ArrayList; import java.util.List; diff --git a/modules/cache-common/src/test/java/org/opensearch/common/cache/policy/TookTimePolicyTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/policy/TookTimePolicyTests.java similarity index 96% rename from modules/cache-common/src/test/java/org/opensearch/common/cache/policy/TookTimePolicyTests.java rename to modules/cache-common/src/test/java/org/opensearch/cache/common/policy/TookTimePolicyTests.java index 4d05c1d13fb59..000067280e50d 100644 --- a/modules/cache-common/src/test/java/org/opensearch/common/cache/policy/TookTimePolicyTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/policy/TookTimePolicyTests.java @@ -6,13 +6,14 @@ * compatible open source license. */ -package org.opensearch.common.cache.policy; +package org.opensearch.cache.common.policy; import org.apache.lucene.search.ScoreDoc; import org.apache.lucene.search.TopDocs; import org.apache.lucene.search.TotalHits; import org.opensearch.common.Randomness; import org.opensearch.common.cache.CacheType; +import org.opensearch.common.cache.policy.CachedQueryResult; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.lucene.search.TopDocsAndMaxScore; import org.opensearch.common.settings.ClusterSettings; @@ -30,7 +31,7 @@ import java.util.Random; import java.util.function.Function; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; public class TookTimePolicyTests extends OpenSearchTestCase { private final Function transformationFunction = (data) -> { diff --git a/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/MockDiskCache.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java similarity index 99% rename from modules/cache-common/src/test/java/org/opensearch/common/cache/tier/MockDiskCache.java rename to modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java index da0885c80e20f..fcddd489a27aa 100644 --- a/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/MockDiskCache.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.ICache; diff --git a/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCachePluginTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCachePluginTests.java similarity index 96% rename from modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCachePluginTests.java rename to modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCachePluginTests.java index 42d397875503e..4a96ffe2069ec 100644 --- a/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCachePluginTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCachePluginTests.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.opensearch.common.cache.ICache; import org.opensearch.common.settings.Settings; diff --git a/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java similarity index 99% rename from modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCacheTests.java rename to modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java index ca8a0f3a4300c..33d8e41718790 100644 --- a/modules/cache-common/src/test/java/org/opensearch/common/cache/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common.cache.tier; +package org.opensearch.cache.common.tier; import org.opensearch.OpenSearchException; import org.opensearch.common.Randomness; @@ -56,17 +56,17 @@ import java.util.function.Function; import java.util.function.Predicate; +import static org.opensearch.cache.common.tier.TieredSpilloverCache.ZERO_SEGMENT_COUNT_EXCEPTION_MESSAGE; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.common.cache.store.settings.OpenSearchOnHeapCacheSettings.MAXIMUM_SIZE_IN_BYTES_KEY; -import static org.opensearch.common.cache.tier.TieredSpilloverCache.ZERO_SEGMENT_COUNT_EXCEPTION_MESSAGE; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; -import static org.opensearch.common.cache.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; From f85b719ec58b7644d2b4d94581d7144498472cc0 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 17:01:05 -0800 Subject: [PATCH 10/12] Addressed Sagar's comments Signed-off-by: Peter Alfonsi --- CHANGELOG.md | 2 +- .../tier/TieredSpilloverCacheStatsIT.java | 6 +-- .../common/tier/TieredSpilloverCache.java | 5 +-- .../TieredSpilloverCacheStatsHolder.java | 17 ++------ .../TieredSpilloverCacheStatsHolderTests.java | 43 +++++++++++++------ .../tier/TieredSpilloverCacheTests.java | 6 +-- .../cache/stats/DefaultCacheStatsHolder.java | 18 ++++---- 7 files changed, 49 insertions(+), 48 deletions(-) rename modules/cache-common/src/main/java/org/opensearch/{common/cache/stats => cache/common/tier}/TieredSpilloverCacheStatsHolder.java (92%) rename modules/cache-common/src/test/java/org/opensearch/{common/cache/stats => cache/common/tier}/TieredSpilloverCacheStatsHolderTests.java (91%) diff --git a/CHANGELOG.md b/CHANGELOG.md index c037708e90091..c28960be4dbec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,7 +82,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix warnings from SLF4J on startup when repository-s3 is installed ([#16194](https://github.com/opensearch-project/OpenSearch/pull/16194)) - Fix protobuf-java leak through client library dependencies ([#16254](https://github.com/opensearch-project/OpenSearch/pull/16254)) - Fix multi-search with template doesn't return status code ([#16265](https://github.com/opensearch-project/OpenSearch/pull/16265)) -- Fix bug in new cache stats API when closing shards while using TieredSpilloverCache ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560)) +- [Tiered Caching] Fix bug in cache stats API ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560)) ### Security diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java index e7f0100f9ea12..a858e94ad1609 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java @@ -38,9 +38,9 @@ import java.util.Map; import java.util.concurrent.TimeUnit; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.indices.IndicesService.INDICES_CACHE_CLEAN_INTERVAL_SETTING; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse; diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java index be777d901d719..0f2dbea710193 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java @@ -21,7 +21,6 @@ import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.policy.CachedQueryResult; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; -import org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder; import org.opensearch.common.cache.store.config.CacheConfig; import org.opensearch.common.collect.Tuple; import org.opensearch.common.settings.Setting; @@ -54,10 +53,10 @@ import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; /** * This cache spillover the evicted items from heap tier to disk tier. All the new items are first cached on heap diff --git a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java similarity index 92% rename from modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java rename to modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java index 76ee9b82e0834..e2778f27d2353 100644 --- a/modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java @@ -6,9 +6,9 @@ * compatible open source license. */ -package org.opensearch.common.cache.stats; +package org.opensearch.cache.common.tier; -import org.opensearch.cache.common.tier.TieredSpilloverCache; +import org.opensearch.common.cache.stats.DefaultCacheStatsHolder; import java.util.ArrayList; import java.util.List; @@ -177,20 +177,9 @@ public void removeDimensions(List dimensionValues) { // As we are removing nodes from the tree, obtain the lock lock.lock(); try { - removeDimensionsHelper(dimensionValues, getStatsRoot(), 0); + removeDimensionsHelper(dimensionValues, statsRoot, 0); } finally { lock.unlock(); } } - - @Override - protected ImmutableCacheStats removeDimensionsBaseCase(Node node) { - // The base case will be the node whose children represent individual tiers. - // Manually delete this node's children - for (String tierValue : TIER_VALUES) { - node.children.remove(tierValue); - } - // Pass up a snapshot of the original stats to avoid issues when the original is decremented by other fn invocations - return node.getImmutableStats(); - } } diff --git a/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java similarity index 91% rename from modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java rename to modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java index 79b86c5f6d080..09273a0761663 100644 --- a/modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java @@ -6,10 +6,12 @@ * compatible open source license. */ -package org.opensearch.common.cache.stats; +package org.opensearch.cache.common.tier; import org.opensearch.common.Randomness; -import org.opensearch.common.metrics.CounterMetric; +import org.opensearch.common.cache.stats.CacheStats; +import org.opensearch.common.cache.stats.DefaultCacheStatsHolder; +import org.opensearch.common.cache.stats.ImmutableCacheStats; import org.opensearch.test.OpenSearchTestCase; import java.util.ArrayList; @@ -21,9 +23,9 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_VALUES; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_VALUES; public class TieredSpilloverCacheStatsHolderTests extends OpenSearchTestCase { // These are modified from DefaultCacheStatsHolderTests.java to account for the tiers. Because we can't add a dependency on server.test, @@ -78,12 +80,17 @@ public void testReset() throws Exception { cacheStatsHolder.reset(); for (List dimensionValues : expected.keySet()) { CacheStats originalCounter = expected.get(dimensionValues); - originalCounter.sizeInBytes = new CounterMetric(); - originalCounter.items = new CounterMetric(); + ImmutableCacheStats expectedTotal = new ImmutableCacheStats( + originalCounter.getHits(), + originalCounter.getMisses(), + originalCounter.getEvictions(), + 0, + 0 + ); DefaultCacheStatsHolder.Node node = getNode(dimensionValues, cacheStatsHolder.getStatsRoot()); ImmutableCacheStats actual = node.getImmutableStats(); - assertEquals(originalCounter.immutableSnapshot(), actual); + assertEquals(expectedTotal, actual); } } @@ -130,7 +137,7 @@ public void testDropStatsForDimensions() throws Exception { // 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(0, cacheStatsHolder.getStatsRoot().children.size()); + // assertEquals(0, cacheStatsHolder.getStatsRoot().getChildren().size()); } } @@ -280,11 +287,19 @@ static Map, CacheStats> populateStats( threadRand.nextInt(5000), threadRand.nextInt(10) ); - expected.get(dimensions).hits.inc(statsToInc.getHits()); - expected.get(dimensions).misses.inc(statsToInc.getMisses()); - expected.get(dimensions).evictions.inc(statsToInc.getEvictions()); - expected.get(dimensions).sizeInBytes.inc(statsToInc.getSizeInBytes()); - expected.get(dimensions).items.inc(statsToInc.getItems()); + for (int iter = 0; iter < statsToInc.getHits(); iter++) { + expected.get(dimensions).incrementHits(); + } + for (int iter = 0; iter < statsToInc.getMisses(); iter++) { + expected.get(dimensions).incrementMisses(); + } + for (int iter = 0; iter < statsToInc.getEvictions(); iter++) { + expected.get(dimensions).incrementEvictions(); + } + expected.get(dimensions).incrementSizeInBytes(statsToInc.getSizeInBytes()); + for (int iter = 0; iter < statsToInc.getItems(); iter++) { + expected.get(dimensions).incrementItems(); + } populateStatsHolderFromStatsValueMap(cacheStatsHolder, Map.of(dimensions, statsToInc), diskTierEnabled); } } diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java index 33d8e41718790..3bb1321f9faf2 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java @@ -61,11 +61,11 @@ import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK; -import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP; import static org.opensearch.common.cache.store.settings.OpenSearchOnHeapCacheSettings.MAXIMUM_SIZE_IN_BYTES_KEY; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; 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 0cec60c479b4e..0148144ecbe8c 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 @@ -37,7 +37,7 @@ public class DefaultCacheStatsHolder implements CacheStatsHolder { // Non-leaf nodes have stats matching the sum of their children. // We use a tree structure, rather than a map with concatenated keys, to save on memory usage. If there are many leaf // nodes that share a parent, that parent's dimension value will only be stored once, not many times. - private final Node statsRoot; + protected final Node statsRoot; // To avoid sync problems, obtain a lock before creating or removing nodes in the stats tree. // No lock is needed to edit stats on existing nodes. protected final Lock lock = new ReentrantLock(); @@ -190,8 +190,10 @@ public void removeDimensions(List dimensionValues) { // Returns a CacheStatsCounterSnapshot object for the stats to decrement if the removal happened, null otherwise. protected ImmutableCacheStats removeDimensionsHelper(List dimensionValues, Node node, int depth) { if (depth == dimensionValues.size()) { + // Remove children, if present. + node.children.clear(); // Pass up a snapshot of the original stats to avoid issues when the original is decremented by other fn invocations - return removeDimensionsBaseCase(node); + return node.getImmutableStats(); } Node child = node.getChild(dimensionValues.get(depth)); if (child == null) { @@ -208,19 +210,15 @@ protected ImmutableCacheStats removeDimensionsHelper(List dimensionValue return statsToDecrement; } - protected ImmutableCacheStats removeDimensionsBaseCase(Node node) { - return node.getImmutableStats(); - } - // pkg-private for testing - Node getStatsRoot() { + public Node getStatsRoot() { return statsRoot; } /** * Nodes that make up the tree in the stats holder. */ - protected static class Node { + public static class Node { private final String dimensionValue; // Map from dimensionValue to the DimensionNode for that dimension value. final Map children; @@ -245,7 +243,7 @@ public String getDimensionValue() { return dimensionValue; } - protected Map getChildren() { + public Map getChildren() { // We can safely iterate over ConcurrentHashMap without worrying about thread issues. return children; } @@ -284,7 +282,7 @@ long getEntries() { return this.stats.getItems(); } - ImmutableCacheStats getImmutableStats() { + public ImmutableCacheStats getImmutableStats() { return this.stats.immutableSnapshot(); } From 165850e06d30b243713bf0f93011370572d55a15 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 5 Nov 2024 09:28:06 -0800 Subject: [PATCH 11/12] More package fixes Signed-off-by: Peter Alfonsi --- .../cache/common/tier/TieredSpilloverCacheStatsHolder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java index e2778f27d2353..7ea6d3504a52c 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java @@ -67,7 +67,7 @@ private static List getDimensionNamesWithTier(List dimensionName /** * Add tierValue to the end of a copy of the initial dimension values, so they can appropriately be used in this stats holder. */ - public List getDimensionsWithTierValue(List initialDimensions, String tierValue) { + List getDimensionsWithTierValue(List initialDimensions, String tierValue) { List result = new ArrayList<>(initialDimensions); result.add(tierValue); return result; @@ -166,7 +166,7 @@ public void decrementItems(List dimensionValues) { super.decrementItems(dimensionValues); } - public void setDiskCacheEnabled(boolean diskCacheEnabled) { + void setDiskCacheEnabled(boolean diskCacheEnabled) { this.diskCacheEnabled = diskCacheEnabled; } From 97c1e72693cef9b6d0fbf9cebea3a1c1cae6263a Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 5 Nov 2024 12:49:32 -0800 Subject: [PATCH 12/12] Addressed andross's comments Signed-off-by: Peter Alfonsi --- .../cache/common/tier/TieredSpilloverCache.java | 8 +++----- .../common/cache/stats/DefaultCacheStatsHolder.java | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java index 0f2dbea710193..38a6915ffd10e 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java @@ -375,12 +375,10 @@ private V compute(ICacheKey key, LoadAwareCacheLoader, V> loader public void invalidate(ICacheKey key) { if (key.getDropStatsForDimensions()) { statsHolder.removeDimensions(key.dimensions); - } else { + } else if (key.key != null) { for (Map.Entry, TierInfo> cacheEntry : caches.entrySet()) { - if (key.key != null) { - try (ReleasableLock ignore = writeLock.acquire()) { - cacheEntry.getKey().invalidate(key); - } + try (ReleasableLock ignore = writeLock.acquire()) { + cacheEntry.getKey().invalidate(key); } } } 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 0148144ecbe8c..7434283ff6f41 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,7 +210,6 @@ protected ImmutableCacheStats removeDimensionsHelper(List dimensionValue return statsToDecrement; } - // pkg-private for testing public Node getStatsRoot() { return statsRoot; }