From f85b719ec58b7644d2b4d94581d7144498472cc0 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 4 Nov 2024 17:01:05 -0800 Subject: [PATCH] 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(); }