From 5ba6e47a8a7e5b28cc39c03cf992340c7cf5fb4c Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 15 Apr 2024 17:04:21 -0700 Subject: [PATCH 01/15] Adds dummy stats holder Signed-off-by: Peter Alfonsi --- .../common/cache/stats/CacheStatsHolder.java | 13 +++- .../stats/CacheStatsHolderInterface.java | 39 +++++++++++ .../cache/stats/DummyCacheStatsHolder.java | 70 +++++++++++++++++++ 3 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java create mode 100644 server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java diff --git a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java index a8b7c27ef9e79..308b1fba395dd 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java @@ -28,7 +28,7 @@ * * @opensearch.experimental */ -public class CacheStatsHolder { +public class CacheStatsHolder implements CacheStatsHolderInterface { // The list of permitted dimensions. Should be ordered from "outermost" to "innermost", as you would like to // aggregate them in an API response. @@ -53,32 +53,39 @@ public List getDimensionNames() { // For all these increment functions, the dimensions list comes from the key, and contains all dimensions present in dimensionNames. // The order has to match the order given in dimensionNames. + @Override public void incrementHits(List dimensionValues) { internalIncrement(dimensionValues, Node::incrementHits, true); } + @Override public void incrementMisses(List dimensionValues) { internalIncrement(dimensionValues, Node::incrementMisses, true); } + @Override public void incrementEvictions(List dimensionValues) { internalIncrement(dimensionValues, Node::incrementEvictions, true); } + @Override public void incrementSizeInBytes(List dimensionValues, long amountBytes) { internalIncrement(dimensionValues, (node) -> node.incrementSizeInBytes(amountBytes), true); } // For decrements, we should not create nodes if they are absent. This protects us from erroneously decrementing values for keys // which have been entirely deleted, for example in an async removal listener. + @Override public void decrementSizeInBytes(List dimensionValues, long amountBytes) { internalIncrement(dimensionValues, (node) -> node.decrementSizeInBytes(amountBytes), false); } + @Override public void incrementEntries(List dimensionValues) { internalIncrement(dimensionValues, Node::incrementEntries, true); } + @Override public void decrementEntries(List dimensionValues) { internalIncrement(dimensionValues, Node::decrementEntries, false); } @@ -87,6 +94,7 @@ public void decrementEntries(List dimensionValues) { * Reset number of entries and memory size when all keys leave the cache, but don't reset hit/miss/eviction numbers. * This is in line with the behavior of the existing API when caches are cleared. */ + @Override public void reset() { resetHelper(statsRoot); } @@ -98,6 +106,7 @@ private void resetHelper(Node current) { } } + @Override public long count() { // Include this here so caches don't have to create an entire CacheStats object to run count(). return statsRoot.getEntries(); @@ -156,10 +165,12 @@ private boolean internalIncrementHelper( /** * Produce an immutable version of these stats. */ + @Override public ImmutableCacheStatsHolder getImmutableCacheStatsHolder() { return new ImmutableCacheStatsHolder(statsRoot.snapshot(), dimensionNames); } + @Override public void removeDimensions(List dimensionValues) { assert dimensionValues.size() == dimensionNames.size() : "Must specify a value for every dimension when removing from StatsHolder"; // As we are removing nodes from the tree, obtain the lock diff --git a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java new file mode 100644 index 0000000000000..2fdf11e75a332 --- /dev/null +++ b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java @@ -0,0 +1,39 @@ +/* + * 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.common.cache.stats; + +import java.util.List; + +/** + * An abstract class extended by CacheStatsHolder and DummyCacheStatsHolder. + * Can be removed once the pluggable caches feature is no longer experimental. + */ +public interface CacheStatsHolderInterface { + void incrementHits(List dimensionValues); + + void incrementMisses(List dimensionValues); + + void incrementEvictions(List dimensionValues); + + void incrementSizeInBytes(List dimensionValues, long amountBytes); + + void decrementSizeInBytes(List dimensionValues, long amountBytes); + + void incrementEntries(List dimensionValues); + + void decrementEntries(List dimensionValues); + + void reset(); + + long count(); + + void removeDimensions(List dimensionValues); + + ImmutableCacheStatsHolder getImmutableCacheStatsHolder(); +} diff --git a/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java new file mode 100644 index 0000000000000..f12b4ed14bc77 --- /dev/null +++ b/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java @@ -0,0 +1,70 @@ +/* + * 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.common.cache.stats; + +import org.opensearch.common.metrics.CounterMetric; + +import java.util.List; + +/** + * A dummy version of CacheStatsHolder, which cache implementations use when FeatureFlags.PLUGGABLE_CACHES is false. + * Will be removed once pluggable caches is no longer an experimental feature. + * Returns all-zero stats when calling getImmutableCacheStatsHolder(). Does keep track of entries for use in ICache.count(). + */ +public class DummyCacheStatsHolder implements CacheStatsHolderInterface { + private final CounterMetric entries; + private final List dimensionNames; + + public DummyCacheStatsHolder(List dimensionNames) { + this.dimensionNames = dimensionNames; + this.entries = new CounterMetric(); + } + + @Override + public void incrementHits(List dimensionValues) {} + + @Override + public void incrementMisses(List dimensionValues) {} + + @Override + public void incrementEvictions(List dimensionValues) {} + + @Override + public void incrementSizeInBytes(List dimensionValues, long amountBytes) {} + + @Override + public void decrementSizeInBytes(List dimensionValues, long amountBytes) {} + + @Override + public void incrementEntries(List dimensionValues) { + entries.inc(); + } + + @Override + public void decrementEntries(List dimensionValues) { + entries.dec(); + } + + @Override + public void reset() {} + + @Override + public long count() { + return entries.count(); + } + + @Override + public void removeDimensions(List dimensionValues) {} + + @Override + public ImmutableCacheStatsHolder getImmutableCacheStatsHolder() { + ImmutableCacheStatsHolder.Node dummyNode = new ImmutableCacheStatsHolder.Node("", null, new ImmutableCacheStats(0, 0, 0, 0, 0)); + return new ImmutableCacheStatsHolder(dummyNode, dimensionNames); + } +} From bd0c2e539ba2127a4fc0b224445fed9239be335b Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 15 Apr 2024 17:05:01 -0700 Subject: [PATCH 02/15] Integrates dummy stats holder with caches Signed-off-by: Peter Alfonsi --- .../cache/store/disk/EhcacheDiskCache.java | 12 +++- .../store/disk/EhCacheDiskCacheTests.java | 65 +++++++++++++++---- .../cache/store/OpenSearchOnHeapCache.java | 11 +++- .../store/OpenSearchOnHeapCacheTests.java | 38 ++++++++++- 4 files changed, 105 insertions(+), 21 deletions(-) diff --git a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java index 185d51732a116..81515c4d6d6f1 100644 --- a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java +++ b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java @@ -25,6 +25,8 @@ import org.opensearch.common.cache.serializer.ICacheKeySerializer; import org.opensearch.common.cache.serializer.Serializer; import org.opensearch.common.cache.stats.CacheStatsHolder; +import org.opensearch.common.cache.stats.CacheStatsHolderInterface; +import org.opensearch.common.cache.stats.DummyCacheStatsHolder; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.cache.store.builders.ICacheBuilder; import org.opensearch.common.cache.store.config.CacheConfig; @@ -32,6 +34,7 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.io.IOUtils; import java.io.File; @@ -113,7 +116,7 @@ public class EhcacheDiskCache implements ICache { private final Class keyType; private final Class valueType; private final TimeValue expireAfterAccess; - private final CacheStatsHolder cacheStatsHolder; + private final CacheStatsHolderInterface cacheStatsHolder; private final EhCacheEventListener ehCacheEventListener; private final String threadPoolAlias; private final Settings settings; @@ -162,7 +165,12 @@ private EhcacheDiskCache(Builder builder) { this.ehCacheEventListener = new EhCacheEventListener(builder.getRemovalListener(), builder.getWeigher()); this.cache = buildCache(Duration.ofMillis(expireAfterAccess.getMillis()), builder); List dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); - this.cacheStatsHolder = new CacheStatsHolder(dimensionNames); + + if (FeatureFlags.PLUGGABLE_CACHE_SETTING.get(builder.getSettings())) { + this.cacheStatsHolder = new CacheStatsHolder(dimensionNames); + } else { + this.cacheStatsHolder = new DummyCacheStatsHolder(dimensionNames); + } } @SuppressWarnings({ "rawtypes" }) diff --git a/plugins/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java b/plugins/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java index 06ebed08d7525..2bf6eb2275646 100644 --- a/plugins/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java +++ b/plugins/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java @@ -25,6 +25,7 @@ import org.opensearch.common.metrics.CounterMetric; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.common.bytes.CompositeBytesReference; @@ -59,7 +60,7 @@ public class EhCacheDiskCacheTests extends OpenSearchSingleNodeTestCase { private final String dimensionName = "shardId"; public void testBasicGetAndPut() throws IOException { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); ToLongBiFunction, String> weigher = getWeigher(); try (NodeEnvironment env = newNodeEnvironment(settings)) { @@ -142,6 +143,7 @@ public void testBasicGetAndPutUsingFactory() throws IOException { .getKey(), true ) + .put(FeatureFlags.PLUGGABLE_CACHE, true) .build() ) .build(), @@ -173,7 +175,7 @@ public void testBasicGetAndPutUsingFactory() throws IOException { } public void testConcurrentPut() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -223,7 +225,7 @@ public void testConcurrentPut() throws Exception { } public void testEhcacheParallelGets() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -272,7 +274,7 @@ public void testEhcacheParallelGets() throws Exception { } public void testEhcacheKeyIterator() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") .setThreadPoolAlias("ehcacheTest") @@ -312,7 +314,7 @@ public void testEhcacheKeyIterator() throws Exception { } public void testEvictions() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); ToLongBiFunction, String> weigher = getWeigher(); try (NodeEnvironment env = newNodeEnvironment(settings)) { @@ -348,7 +350,7 @@ public void testEvictions() throws Exception { } public void testComputeIfAbsentConcurrently() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -424,7 +426,7 @@ public String load(ICacheKey key) { } public void testComputeIfAbsentConcurrentlyAndThrowsException() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -485,7 +487,7 @@ public String load(ICacheKey key) throws Exception { } public void testComputeIfAbsentWithNullValueLoading() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -552,7 +554,7 @@ public String load(ICacheKey key) throws Exception { public void testMemoryTracking() throws Exception { // Test all cases for EhCacheEventListener.onEvent and check stats memory usage is updated correctly - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); ToLongBiFunction, String> weigher = getWeigher(); int initialKeyLength = 40; int initialValueLength = 40; @@ -626,7 +628,7 @@ public void testMemoryTracking() throws Exception { } public void testEhcacheKeyIteratorWithRemove() throws IOException { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") .setThreadPoolAlias("ehcacheTest") @@ -673,7 +675,7 @@ public void testEhcacheKeyIteratorWithRemove() throws IOException { } public void testInvalidateAll() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") @@ -710,7 +712,7 @@ public void testInvalidateAll() throws Exception { } public void testBasicGetAndPutBytesReference() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehCacheDiskCachingTier = new EhcacheDiskCache.Builder() .setThreadPoolAlias("ehcacheTest") @@ -756,7 +758,7 @@ public void testBasicGetAndPutBytesReference() throws Exception { } public void testInvalidate() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") @@ -800,7 +802,7 @@ public void testInvalidate() throws Exception { // Modified from OpenSearchOnHeapCacheTests.java public void testInvalidateWithDropDimensions() throws Exception { - Settings settings = Settings.builder().build(); + Settings settings = getSettings(true); List dimensionNames = List.of("dim1", "dim2"); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehCacheDiskCachingTier = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") @@ -849,6 +851,37 @@ public void testInvalidateWithDropDimensions() throws Exception { } } + public void testStatsWithoutPluggableCaches() throws Exception { + Settings settings = getSettings(false); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") + .setStoragePath(env.nodePaths()[0].indicesPath.toString() + "/request_cache") + .setIsEventListenerModeSync(true) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .build(); + int randomKeys = randomIntBetween(10, 100); + for (int i = 0; i < randomKeys; i++) { + ICacheKey iCacheKey = getICacheKey(UUID.randomUUID().toString()); + ehcacheTest.put(iCacheKey, UUID.randomUUID().toString()); + assertEquals(i + 1, ehcacheTest.count()); + assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), ehcacheTest.stats().getTotalStats()); + } + ehcacheTest.close(); + } + } + private List getRandomDimensions(List dimensionNames) { Random rand = Randomness.get(); int bound = 3; @@ -893,6 +926,10 @@ private ToLongBiFunction, String> getWeigher() { }; } + private Settings getSettings(boolean pluggableCaches) { + return Settings.builder().put(FeatureFlags.PLUGGABLE_CACHE, pluggableCaches).build(); + } + static class MockRemovalListener implements RemovalListener, V> { CounterMetric evictionMetric = new CounterMetric(); diff --git a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java index 29e5667c9f27d..15cd9e4ba0a71 100644 --- a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java +++ b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java @@ -19,6 +19,8 @@ import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.settings.CacheSettings; import org.opensearch.common.cache.stats.CacheStatsHolder; +import org.opensearch.common.cache.stats.CacheStatsHolderInterface; +import org.opensearch.common.cache.stats.DummyCacheStatsHolder; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.cache.store.builders.ICacheBuilder; import org.opensearch.common.cache.store.config.CacheConfig; @@ -47,7 +49,7 @@ public class OpenSearchOnHeapCache implements ICache, RemovalListener, V> { private final Cache, V> cache; - private final CacheStatsHolder cacheStatsHolder; + private final CacheStatsHolderInterface cacheStatsHolder; private final RemovalListener, V> removalListener; private final List dimensionNames; private final ToLongBiFunction, V> weigher; @@ -62,7 +64,11 @@ public OpenSearchOnHeapCache(Builder builder) { } cache = cacheBuilder.build(); this.dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); - this.cacheStatsHolder = new CacheStatsHolder(dimensionNames); + if (FeatureFlags.PLUGGABLE_CACHE_SETTING.get(builder.getSettings())) { + this.cacheStatsHolder = new CacheStatsHolder(dimensionNames); + } else { + this.cacheStatsHolder = new DummyCacheStatsHolder(dimensionNames); + } this.removalListener = builder.getRemovalListener(); this.weigher = builder.getWeigher(); } @@ -167,6 +173,7 @@ public ICache create(CacheConfig config, CacheType cacheType, .setMaximumWeightInBytes(((ByteSizeValue) settingList.get(MAXIMUM_SIZE_IN_BYTES_KEY).get(settings)).getBytes()) .setExpireAfterAccess(((TimeValue) settingList.get(EXPIRE_AFTER_ACCESS_KEY).get(settings))) .setWeigher(config.getWeigher()) + .setSettings(config.getSettings()) .setRemovalListener(config.getRemovalListener()); Setting cacheSettingForCacheType = CacheSettings.CACHE_TYPE_STORE_NAME.getConcreteSettingForNamespace( cacheType.getSettingPrefix() diff --git a/server/src/test/java/org/opensearch/common/cache/store/OpenSearchOnHeapCacheTests.java b/server/src/test/java/org/opensearch/common/cache/store/OpenSearchOnHeapCacheTests.java index 008dc7c2e0902..00dbf43bc37be 100644 --- a/server/src/test/java/org/opensearch/common/cache/store/OpenSearchOnHeapCacheTests.java +++ b/server/src/test/java/org/opensearch/common/cache/store/OpenSearchOnHeapCacheTests.java @@ -16,10 +16,12 @@ import org.opensearch.common.cache.RemovalListener; import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.stats.ImmutableCacheStats; +import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.cache.store.config.CacheConfig; import org.opensearch.common.cache.store.settings.OpenSearchOnHeapCacheSettings; import org.opensearch.common.metrics.CounterMetric; import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.test.OpenSearchTestCase; import java.util.ArrayList; @@ -37,7 +39,9 @@ public void testStats() throws Exception { MockRemovalListener listener = new MockRemovalListener<>(); int maxKeys = between(10, 50); int numEvicted = between(10, 20); - OpenSearchOnHeapCache cache = getCache(maxKeys, listener); + OpenSearchOnHeapCache cache = getCache(maxKeys, listener, true); + + // When the pluggable caches setting is on, we should get stats as expected from cache.stats(). List> keysAdded = new ArrayList<>(); int numAdded = maxKeys + numEvicted; @@ -77,7 +81,34 @@ public void testStats() throws Exception { } } - private OpenSearchOnHeapCache getCache(int maxSizeKeys, MockRemovalListener listener) { + public void testStatsWithoutPluggableCaches() throws Exception { + // When the pluggable caches setting is off, we should get all-zero stats from cache.stats(), but count() should still work. + MockRemovalListener listener = new MockRemovalListener<>(); + int maxKeys = between(10, 50); + int numEvicted = between(10, 20); + OpenSearchOnHeapCache cache = getCache(maxKeys, listener, false); + + List> keysAdded = new ArrayList<>(); + int numAdded = maxKeys + numEvicted; + for (int i = 0; i < numAdded; i++) { + ICacheKey key = getICacheKey(UUID.randomUUID().toString()); + keysAdded.add(key); + cache.computeIfAbsent(key, getLoadAwareCacheLoader()); + + assertEquals(Math.min(maxKeys, i + 1), cache.count()); + assertZeroStats(cache.stats()); + } + } + + private void assertZeroStats(ImmutableCacheStatsHolder stats) { + assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), stats.getTotalStats()); + } + + private OpenSearchOnHeapCache getCache( + int maxSizeKeys, + MockRemovalListener listener, + boolean pluggableCachesSetting + ) { ICache.Factory onHeapCacheFactory = new OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory(); Settings settings = Settings.builder() .put( @@ -86,6 +117,7 @@ private OpenSearchOnHeapCache getCache(int maxSizeKeys, MockRemo .getKey(), maxSizeKeys * keyValueSize + "b" ) + .put(FeatureFlags.PLUGGABLE_CACHE, pluggableCachesSetting) .build(); CacheConfig cacheConfig = new CacheConfig.Builder().setKeyType(String.class) @@ -102,7 +134,7 @@ private OpenSearchOnHeapCache getCache(int maxSizeKeys, MockRemo public void testInvalidateWithDropDimensions() throws Exception { MockRemovalListener listener = new MockRemovalListener<>(); int maxKeys = 50; - OpenSearchOnHeapCache cache = getCache(maxKeys, listener); + OpenSearchOnHeapCache cache = getCache(maxKeys, listener, true); List> keysAdded = new ArrayList<>(); From 8d2914e90aa894f9a4627d6d98769e457ca2fc12 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 16 Apr 2024 09:52:56 -0700 Subject: [PATCH 03/15] Changelog Signed-off-by: Peter Alfonsi --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88a8f57c0afdc..de6c564c0231d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Tiered Caching] Add dimension-based stats to ICache implementations. ([#12531](https://github.com/opensearch-project/OpenSearch/pull/12531)) - Add changes for overriding remote store and replication settings during snapshot restore. ([#11868](https://github.com/opensearch-project/OpenSearch/pull/11868)) - Add an individual setting of rate limiter for segment replication ([#12959](https://github.com/opensearch-project/OpenSearch/pull/12959)) +- [Tiered Caching] Gate new stats logic behind FeatureFlags.PLUGGABLE_CACHE ([#13238](https://github.com/opensearch-project/OpenSearch/pull/13238)) ### Dependencies - Bump `org.apache.commons:commons-configuration2` from 2.10.0 to 2.10.1 ([#12896](https://github.com/opensearch-project/OpenSearch/pull/12896)) From 3382d635305f10c79dc02c3c09afcbbdc43ff4ac Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 16 Apr 2024 14:41:49 -0700 Subject: [PATCH 04/15] Fixed failing tests in IndicesRequestCacheTests Signed-off-by: Peter Alfonsi --- .../cache/stats/DummyCacheStatsHolder.java | 18 ++++++++++++++---- .../indices/IndicesRequestCache.java | 7 ------- .../indices/IndicesRequestCacheTests.java | 12 ++++++++---- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java index f12b4ed14bc77..26910efb43693 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java @@ -18,12 +18,14 @@ * Returns all-zero stats when calling getImmutableCacheStatsHolder(). Does keep track of entries for use in ICache.count(). */ public class DummyCacheStatsHolder implements CacheStatsHolderInterface { - private final CounterMetric entries; + private CounterMetric entries; + private CounterMetric sizeInBytes; private final List dimensionNames; public DummyCacheStatsHolder(List dimensionNames) { this.dimensionNames = dimensionNames; this.entries = new CounterMetric(); + this.sizeInBytes = new CounterMetric(); } @Override @@ -36,10 +38,14 @@ public void incrementMisses(List dimensionValues) {} public void incrementEvictions(List dimensionValues) {} @Override - public void incrementSizeInBytes(List dimensionValues, long amountBytes) {} + public void incrementSizeInBytes(List dimensionValues, long amountBytes) { + sizeInBytes.inc(amountBytes); + } @Override - public void decrementSizeInBytes(List dimensionValues, long amountBytes) {} + public void decrementSizeInBytes(List dimensionValues, long amountBytes) { + sizeInBytes.dec(amountBytes); + } @Override public void incrementEntries(List dimensionValues) { @@ -52,7 +58,10 @@ public void decrementEntries(List dimensionValues) { } @Override - public void reset() {} + public void reset() { + this.entries = new CounterMetric(); + this.sizeInBytes = new CounterMetric(); + } @Override public long count() { @@ -64,6 +73,7 @@ public void removeDimensions(List dimensionValues) {} @Override public ImmutableCacheStatsHolder getImmutableCacheStatsHolder() { + // TODO: can we pass all-zero without adding dependency on API PR (bc of test-only IRC.getSizeInBytes()) ? ImmutableCacheStatsHolder.Node dummyNode = new ImmutableCacheStatsHolder.Node("", null, new ImmutableCacheStats(0, 0, 0, 0, 0)); return new ImmutableCacheStatsHolder(dummyNode, dimensionNames); } diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index eab772cda3213..d5ec7bc609584 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -756,13 +756,6 @@ long count() { return cache.count(); } - /** - * Returns the current size in bytes of the cache - */ - long getSizeInBytes() { - return cache.stats().getTotalSizeInBytes(); - } - /** * Returns the current cache stats. Pkg-private for testing. */ diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index e3dca1b7bfda2..29c796ed7da30 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -803,7 +803,10 @@ public void testClosingIndexWipesStats() throws Exception { assertNotNull(indexToClose.getShard(i)); } ThreadPool threadPool = getThreadPool(); - Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.001%").build(); + Settings settings = Settings.builder() + .put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.001%") + .put(FeatureFlags.PLUGGABLE_CACHE, true) + .build(); IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { IndexService indexService = null; try { @@ -930,15 +933,16 @@ public void testEviction() throws Exception { assertEquals("foo", value1.streamInput().readString()); BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes); assertEquals("bar", value2.streamInput().readString()); - size = new ByteSizeValue(cache.getSizeInBytes()); + size = indexShard.requestCache().stats().getMemorySize(); // Value from old API IOUtils.close(reader, secondReader, writer, dir, cache); terminate(threadPool); } IndexShard indexShard = createIndex("test1").getShard(0); ThreadPool threadPool = getThreadPool(); IndicesRequestCache cache = new IndicesRequestCache( - // Add 5 instead of 1; the key size now depends on the length of dimension names and values so there's more variation - Settings.builder().put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), size.getBytes() + 5 + "b").build(), + // TODO: Add wiggle room to max size to allow for overhead of ICacheKey. This can be removed once API PR goes in, as it updates + // the old API to account for the ICacheKey overhead. + Settings.builder().put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), (int) (size.getBytes() * 1.2) + "b").build(), (shardId -> Optional.of(new IndicesService.IndexShardCacheEntity(indexShard))), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool, From 262e854f2cac5ab6b1bbacb1d01ca3acaf958216 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 17 Apr 2024 12:33:25 -0700 Subject: [PATCH 05/15] cleanup Signed-off-by: Peter Alfonsi --- .../common/cache/stats/CacheStatsHolderInterface.java | 2 +- .../opensearch/common/cache/stats/DummyCacheStatsHolder.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java index 2fdf11e75a332..0b291ab74b3b1 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java @@ -11,7 +11,7 @@ import java.util.List; /** - * An abstract class extended by CacheStatsHolder and DummyCacheStatsHolder. + * An interface extended by CacheStatsHolder and DummyCacheStatsHolder. * Can be removed once the pluggable caches feature is no longer experimental. */ public interface CacheStatsHolderInterface { diff --git a/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java index 26910efb43693..d397cb60c15bf 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java @@ -73,7 +73,6 @@ public void removeDimensions(List dimensionValues) {} @Override public ImmutableCacheStatsHolder getImmutableCacheStatsHolder() { - // TODO: can we pass all-zero without adding dependency on API PR (bc of test-only IRC.getSizeInBytes()) ? ImmutableCacheStatsHolder.Node dummyNode = new ImmutableCacheStatsHolder.Node("", null, new ImmutableCacheStats(0, 0, 0, 0, 0)); return new ImmutableCacheStatsHolder(dummyNode, dimensionNames); } From f38be83ba048d477002f9bad48d8e37a335d5788 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 18 Apr 2024 11:26:37 -0700 Subject: [PATCH 06/15] Addressed Sagar's comments Signed-off-by: Peter Alfonsi --- .../cache/store/disk/EhcacheDiskCache.java | 14 +- .../store/disk/EhCacheDiskCacheTests.java | 65 +--- .../common/cache/stats/CacheStatsHolder.java | 295 +---------------- .../stats/CacheStatsHolderInterface.java | 39 --- .../cache/stats/DefaultCacheStatsHolder.java | 306 ++++++++++++++++++ ...sHolder.java => NoopCacheStatsHolder.java} | 19 +- .../cache/store/OpenSearchOnHeapCache.java | 10 +- ...java => DefaultCacheStatsHolderTests.java} | 43 +-- .../stats/ImmutableCacheStatsHolderTests.java | 23 +- 9 files changed, 387 insertions(+), 427 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java create mode 100644 server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java rename server/src/main/java/org/opensearch/common/cache/stats/{DummyCacheStatsHolder.java => NoopCacheStatsHolder.java} (76%) rename server/src/test/java/org/opensearch/common/cache/stats/{CacheStatsHolderTests.java => DefaultCacheStatsHolderTests.java} (85%) diff --git a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java index 81515c4d6d6f1..eea13ce70ccb5 100644 --- a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java +++ b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java @@ -25,8 +25,7 @@ import org.opensearch.common.cache.serializer.ICacheKeySerializer; import org.opensearch.common.cache.serializer.Serializer; import org.opensearch.common.cache.stats.CacheStatsHolder; -import org.opensearch.common.cache.stats.CacheStatsHolderInterface; -import org.opensearch.common.cache.stats.DummyCacheStatsHolder; +import org.opensearch.common.cache.stats.DefaultCacheStatsHolder; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.cache.store.builders.ICacheBuilder; import org.opensearch.common.cache.store.config.CacheConfig; @@ -34,7 +33,6 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.io.IOUtils; import java.io.File; @@ -116,7 +114,7 @@ public class EhcacheDiskCache implements ICache { private final Class keyType; private final Class valueType; private final TimeValue expireAfterAccess; - private final CacheStatsHolderInterface cacheStatsHolder; + private final CacheStatsHolder cacheStatsHolder; private final EhCacheEventListener ehCacheEventListener; private final String threadPoolAlias; private final Settings settings; @@ -165,12 +163,8 @@ private EhcacheDiskCache(Builder builder) { this.ehCacheEventListener = new EhCacheEventListener(builder.getRemovalListener(), builder.getWeigher()); this.cache = buildCache(Duration.ofMillis(expireAfterAccess.getMillis()), builder); List dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); - - if (FeatureFlags.PLUGGABLE_CACHE_SETTING.get(builder.getSettings())) { - this.cacheStatsHolder = new CacheStatsHolder(dimensionNames); - } else { - this.cacheStatsHolder = new DummyCacheStatsHolder(dimensionNames); - } + // If this cache is being used, FeatureFlags.PLUGGABLE_CACHE is already on, so we can always use the DefaultCacheStatsHolder. + this.cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); } @SuppressWarnings({ "rawtypes" }) diff --git a/plugins/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java b/plugins/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java index 2bf6eb2275646..06ebed08d7525 100644 --- a/plugins/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java +++ b/plugins/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java @@ -25,7 +25,6 @@ import org.opensearch.common.metrics.CounterMetric; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.common.bytes.CompositeBytesReference; @@ -60,7 +59,7 @@ public class EhCacheDiskCacheTests extends OpenSearchSingleNodeTestCase { private final String dimensionName = "shardId"; public void testBasicGetAndPut() throws IOException { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); ToLongBiFunction, String> weigher = getWeigher(); try (NodeEnvironment env = newNodeEnvironment(settings)) { @@ -143,7 +142,6 @@ public void testBasicGetAndPutUsingFactory() throws IOException { .getKey(), true ) - .put(FeatureFlags.PLUGGABLE_CACHE, true) .build() ) .build(), @@ -175,7 +173,7 @@ public void testBasicGetAndPutUsingFactory() throws IOException { } public void testConcurrentPut() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -225,7 +223,7 @@ public void testConcurrentPut() throws Exception { } public void testEhcacheParallelGets() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -274,7 +272,7 @@ public void testEhcacheParallelGets() throws Exception { } public void testEhcacheKeyIterator() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") .setThreadPoolAlias("ehcacheTest") @@ -314,7 +312,7 @@ public void testEhcacheKeyIterator() throws Exception { } public void testEvictions() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); ToLongBiFunction, String> weigher = getWeigher(); try (NodeEnvironment env = newNodeEnvironment(settings)) { @@ -350,7 +348,7 @@ public void testEvictions() throws Exception { } public void testComputeIfAbsentConcurrently() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -426,7 +424,7 @@ public String load(ICacheKey key) { } public void testComputeIfAbsentConcurrentlyAndThrowsException() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -487,7 +485,7 @@ public String load(ICacheKey key) throws Exception { } public void testComputeIfAbsentWithNullValueLoading() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") @@ -554,7 +552,7 @@ public String load(ICacheKey key) throws Exception { public void testMemoryTracking() throws Exception { // Test all cases for EhCacheEventListener.onEvent and check stats memory usage is updated correctly - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); ToLongBiFunction, String> weigher = getWeigher(); int initialKeyLength = 40; int initialValueLength = 40; @@ -628,7 +626,7 @@ public void testMemoryTracking() throws Exception { } public void testEhcacheKeyIteratorWithRemove() throws IOException { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setDiskCacheAlias("test1") .setThreadPoolAlias("ehcacheTest") @@ -675,7 +673,7 @@ public void testEhcacheKeyIteratorWithRemove() throws IOException { } public void testInvalidateAll() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") @@ -712,7 +710,7 @@ public void testInvalidateAll() throws Exception { } public void testBasicGetAndPutBytesReference() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehCacheDiskCachingTier = new EhcacheDiskCache.Builder() .setThreadPoolAlias("ehcacheTest") @@ -758,7 +756,7 @@ public void testBasicGetAndPutBytesReference() throws Exception { } public void testInvalidate() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") @@ -802,7 +800,7 @@ public void testInvalidate() throws Exception { // Modified from OpenSearchOnHeapCacheTests.java public void testInvalidateWithDropDimensions() throws Exception { - Settings settings = getSettings(true); + Settings settings = Settings.builder().build(); List dimensionNames = List.of("dim1", "dim2"); try (NodeEnvironment env = newNodeEnvironment(settings)) { ICache ehCacheDiskCachingTier = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") @@ -851,37 +849,6 @@ public void testInvalidateWithDropDimensions() throws Exception { } } - public void testStatsWithoutPluggableCaches() throws Exception { - Settings settings = getSettings(false); - MockRemovalListener removalListener = new MockRemovalListener<>(); - ToLongBiFunction, String> weigher = getWeigher(); - try (NodeEnvironment env = newNodeEnvironment(settings)) { - ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") - .setStoragePath(env.nodePaths()[0].indicesPath.toString() + "/request_cache") - .setIsEventListenerModeSync(true) - .setKeyType(String.class) - .setValueType(String.class) - .setKeySerializer(new StringSerializer()) - .setValueSerializer(new StringSerializer()) - .setDimensionNames(List.of(dimensionName)) - .setCacheType(CacheType.INDICES_REQUEST_CACHE) - .setSettings(settings) - .setExpireAfterAccess(TimeValue.MAX_VALUE) - .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) - .setRemovalListener(removalListener) - .setWeigher(weigher) - .build(); - int randomKeys = randomIntBetween(10, 100); - for (int i = 0; i < randomKeys; i++) { - ICacheKey iCacheKey = getICacheKey(UUID.randomUUID().toString()); - ehcacheTest.put(iCacheKey, UUID.randomUUID().toString()); - assertEquals(i + 1, ehcacheTest.count()); - assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), ehcacheTest.stats().getTotalStats()); - } - ehcacheTest.close(); - } - } - private List getRandomDimensions(List dimensionNames) { Random rand = Randomness.get(); int bound = 3; @@ -926,10 +893,6 @@ private ToLongBiFunction, String> getWeigher() { }; } - private Settings getSettings(boolean pluggableCaches) { - return Settings.builder().put(FeatureFlags.PLUGGABLE_CACHE, pluggableCaches).build(); - } - static class MockRemovalListener implements RemovalListener, V> { CounterMetric evictionMetric = new CounterMetric(); diff --git a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java index 308b1fba395dd..cdc910117b862 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java @@ -8,299 +8,32 @@ package org.opensearch.common.cache.stats; -import java.util.Collections; -import java.util.HashMap; import java.util.List; -import java.util.Map; -import java.util.TreeMap; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; -import java.util.function.Consumer; /** - * A class ICache implementations use to internally keep track of their stats across multiple dimensions. - * Not intended to be exposed outside the cache; for this, caches use getImmutableCacheStatsHolder() to create an immutable - * copy of the current state of the stats. - * Currently, in the IRC, the stats tracked in a CacheStatsHolder will not appear for empty shards that have had no cache - * operations done on them yet. This might be changed in the future, by exposing a method to add empty nodes to the - * tree in CacheStatsHolder in the ICache interface. - * - * @opensearch.experimental + * An interface extended by DefaultCacheStatsHolder and NoopCacheStatsHolder. + * Can be removed once the pluggable caches feature is no longer experimental. */ -public class CacheStatsHolder implements CacheStatsHolderInterface { - - // The list of permitted dimensions. Should be ordered from "outermost" to "innermost", as you would like to - // aggregate them in an API response. - private final List dimensionNames; - // A tree structure based on dimension values, which stores stats values in its leaf nodes. - // 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; - // 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(); - - public CacheStatsHolder(List dimensionNames) { - this.dimensionNames = Collections.unmodifiableList(dimensionNames); - this.statsRoot = new Node("", true); // The root node has the empty string as its dimension value - } - - public List getDimensionNames() { - return dimensionNames; - } - - // For all these increment functions, the dimensions list comes from the key, and contains all dimensions present in dimensionNames. - // The order has to match the order given in dimensionNames. - @Override - public void incrementHits(List dimensionValues) { - internalIncrement(dimensionValues, Node::incrementHits, true); - } - - @Override - public void incrementMisses(List dimensionValues) { - internalIncrement(dimensionValues, Node::incrementMisses, true); - } - - @Override - public void incrementEvictions(List dimensionValues) { - internalIncrement(dimensionValues, Node::incrementEvictions, true); - } - - @Override - public void incrementSizeInBytes(List dimensionValues, long amountBytes) { - internalIncrement(dimensionValues, (node) -> node.incrementSizeInBytes(amountBytes), true); - } - - // For decrements, we should not create nodes if they are absent. This protects us from erroneously decrementing values for keys - // which have been entirely deleted, for example in an async removal listener. - @Override - public void decrementSizeInBytes(List dimensionValues, long amountBytes) { - internalIncrement(dimensionValues, (node) -> node.decrementSizeInBytes(amountBytes), false); - } - - @Override - public void incrementEntries(List dimensionValues) { - internalIncrement(dimensionValues, Node::incrementEntries, true); - } - - @Override - public void decrementEntries(List dimensionValues) { - internalIncrement(dimensionValues, Node::decrementEntries, false); - } - - /** - * Reset number of entries and memory size when all keys leave the cache, but don't reset hit/miss/eviction numbers. - * This is in line with the behavior of the existing API when caches are cleared. - */ - @Override - public void reset() { - resetHelper(statsRoot); - } - - private void resetHelper(Node current) { - current.resetSizeAndEntries(); - for (Node child : current.children.values()) { - resetHelper(child); - } - } - - @Override - public long count() { - // Include this here so caches don't have to create an entire CacheStats object to run count(). - return statsRoot.getEntries(); - } - - private void internalIncrement(List dimensionValues, Consumer adder, boolean createNodesIfAbsent) { - assert dimensionValues.size() == dimensionNames.size(); - // First try to increment without creating nodes - boolean didIncrement = internalIncrementHelper(dimensionValues, statsRoot, 0, adder, false); - // If we failed to increment, because nodes had to be created, obtain the lock and run again while creating nodes if needed - if (!didIncrement && createNodesIfAbsent) { - try { - lock.lock(); - internalIncrementHelper(dimensionValues, statsRoot, 0, adder, true); - } finally { - lock.unlock(); - } - } - } - - /** - * Use the incrementer function to increment/decrement a value in the stats for a set of dimensions. - * If createNodesIfAbsent is true, and there is no stats for this set of dimensions, create one. - * Returns true if the increment was applied, false if not. - */ - private boolean internalIncrementHelper( - List dimensionValues, - Node node, - int depth, // Pass in the depth to avoid having to slice the list for each node. - Consumer adder, - boolean createNodesIfAbsent - ) { - if (depth == dimensionValues.size()) { - // This is the leaf node we are trying to reach - adder.accept(node); - return true; - } - - Node child = node.getChild(dimensionValues.get(depth)); - if (child == null) { - if (createNodesIfAbsent) { - boolean createMapInChild = depth < dimensionValues.size() - 1; - child = node.createChild(dimensionValues.get(depth), createMapInChild); - } else { - return false; - } - } - if (internalIncrementHelper(dimensionValues, child, depth + 1, adder, createNodesIfAbsent)) { - // Function returns true if the next node down was incremented - adder.accept(node); - return true; - } - return false; - } - - /** - * Produce an immutable version of these stats. - */ - @Override - public ImmutableCacheStatsHolder getImmutableCacheStatsHolder() { - return new ImmutableCacheStatsHolder(statsRoot.snapshot(), dimensionNames); - } - - @Override - public void removeDimensions(List dimensionValues) { - assert dimensionValues.size() == dimensionNames.size() : "Must specify a value for every dimension when removing from StatsHolder"; - // As we are removing nodes from the tree, obtain the lock - lock.lock(); - try { - removeDimensionsHelper(dimensionValues, statsRoot, 0); - } finally { - lock.unlock(); - } - } - - // Returns a CacheStatsCounterSnapshot object for the stats to decrement if the removal happened, null otherwise. - private 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(); - } - 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; - } - - // pkg-private for testing - Node getStatsRoot() { - return statsRoot; - } - - static class Node { - private final String dimensionValue; - // Map from dimensionValue to the DimensionNode for that dimension value. - 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; - - // Used for leaf nodes to avoid allocating many unnecessary maps - private static final Map EMPTY_CHILDREN_MAP = new HashMap<>(); - - Node(String dimensionValue, boolean createChildrenMap) { - this.dimensionValue = dimensionValue; - if (createChildrenMap) { - this.children = new ConcurrentHashMap<>(); - } else { - this.children = EMPTY_CHILDREN_MAP; - } - this.stats = new CacheStats(); - } - - public String getDimensionValue() { - return dimensionValue; - } - - protected Map getChildren() { - // We can safely iterate over ConcurrentHashMap without worrying about thread issues. - return children; - } - - // Functions for modifying internal CacheStatsCounter without callers having to be aware of CacheStatsCounter - - void incrementHits() { - this.stats.incrementHits(); - } - - void incrementMisses() { - this.stats.incrementMisses(); - } - - void incrementEvictions() { - this.stats.incrementEvictions(); - } - - void incrementSizeInBytes(long amountBytes) { - this.stats.incrementSizeInBytes(amountBytes); - } +public interface CacheStatsHolder { + void incrementHits(List dimensionValues); - void decrementSizeInBytes(long amountBytes) { - this.stats.decrementSizeInBytes(amountBytes); - } + void incrementMisses(List dimensionValues); - void incrementEntries() { - this.stats.incrementEntries(); - } + void incrementEvictions(List dimensionValues); - void decrementEntries() { - this.stats.decrementEntries(); - } + void incrementSizeInBytes(List dimensionValues, long amountBytes); - long getEntries() { - return this.stats.getEntries(); - } + void decrementSizeInBytes(List dimensionValues, long amountBytes); - ImmutableCacheStats getImmutableStats() { - return this.stats.immutableSnapshot(); - } + void incrementEntries(List dimensionValues); - void decrementBySnapshot(ImmutableCacheStats snapshot) { - this.stats.subtract(snapshot); - } + void decrementEntries(List dimensionValues); - void resetSizeAndEntries() { - this.stats.resetSizeAndEntries(); - } + void reset(); - Node getChild(String dimensionValue) { - return children.get(dimensionValue); - } + long count(); - Node createChild(String dimensionValue, boolean createMapInChild) { - return children.computeIfAbsent(dimensionValue, (key) -> new Node(dimensionValue, createMapInChild)); - } + void removeDimensions(List dimensionValues); - ImmutableCacheStatsHolder.Node snapshot() { - TreeMap snapshotChildren = null; - if (!children.isEmpty()) { - snapshotChildren = new TreeMap<>(); - for (Node child : children.values()) { - snapshotChildren.put(child.getDimensionValue(), child.snapshot()); - } - } - return new ImmutableCacheStatsHolder.Node(dimensionValue, snapshotChildren, getImmutableStats()); - } - } + ImmutableCacheStatsHolder getImmutableCacheStatsHolder(); } diff --git a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java deleted file mode 100644 index 0b291ab74b3b1..0000000000000 --- a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolderInterface.java +++ /dev/null @@ -1,39 +0,0 @@ -/* - * 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.common.cache.stats; - -import java.util.List; - -/** - * An interface extended by CacheStatsHolder and DummyCacheStatsHolder. - * Can be removed once the pluggable caches feature is no longer experimental. - */ -public interface CacheStatsHolderInterface { - void incrementHits(List dimensionValues); - - void incrementMisses(List dimensionValues); - - void incrementEvictions(List dimensionValues); - - void incrementSizeInBytes(List dimensionValues, long amountBytes); - - void decrementSizeInBytes(List dimensionValues, long amountBytes); - - void incrementEntries(List dimensionValues); - - void decrementEntries(List dimensionValues); - - void reset(); - - long count(); - - void removeDimensions(List dimensionValues); - - ImmutableCacheStatsHolder getImmutableCacheStatsHolder(); -} 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 new file mode 100644 index 0000000000000..ad943e0b2ed1a --- /dev/null +++ b/server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java @@ -0,0 +1,306 @@ +/* + * 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.common.cache.stats; + +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.function.Consumer; + +/** + * A class ICache implementations use to internally keep track of their stats across multiple dimensions. + * Not intended to be exposed outside the cache; for this, caches use getImmutableCacheStatsHolder() to create an immutable + * copy of the current state of the stats. + * Currently, in the IRC, the stats tracked in a CacheStatsHolder will not appear for empty shards that have had no cache + * operations done on them yet. This might be changed in the future, by exposing a method to add empty nodes to the + * tree in CacheStatsHolder in the ICache interface. + * + * @opensearch.experimental + */ +public class DefaultCacheStatsHolder implements CacheStatsHolder { + + // The list of permitted dimensions. Should be ordered from "outermost" to "innermost", as you would like to + // aggregate them in an API response. + private final List dimensionNames; + // A tree structure based on dimension values, which stores stats values in its leaf nodes. + // 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; + // 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(); + + public DefaultCacheStatsHolder(List dimensionNames) { + this.dimensionNames = Collections.unmodifiableList(dimensionNames); + this.statsRoot = new Node("", true); // The root node has the empty string as its dimension value + } + + public List getDimensionNames() { + return dimensionNames; + } + + // For all these increment functions, the dimensions list comes from the key, and contains all dimensions present in dimensionNames. + // The order has to match the order given in dimensionNames. + @Override + public void incrementHits(List dimensionValues) { + internalIncrement(dimensionValues, Node::incrementHits, true); + } + + @Override + public void incrementMisses(List dimensionValues) { + internalIncrement(dimensionValues, Node::incrementMisses, true); + } + + @Override + public void incrementEvictions(List dimensionValues) { + internalIncrement(dimensionValues, Node::incrementEvictions, true); + } + + @Override + public void incrementSizeInBytes(List dimensionValues, long amountBytes) { + internalIncrement(dimensionValues, (node) -> node.incrementSizeInBytes(amountBytes), true); + } + + // For decrements, we should not create nodes if they are absent. This protects us from erroneously decrementing values for keys + // which have been entirely deleted, for example in an async removal listener. + @Override + public void decrementSizeInBytes(List dimensionValues, long amountBytes) { + internalIncrement(dimensionValues, (node) -> node.decrementSizeInBytes(amountBytes), false); + } + + @Override + public void incrementEntries(List dimensionValues) { + internalIncrement(dimensionValues, Node::incrementEntries, true); + } + + @Override + public void decrementEntries(List dimensionValues) { + internalIncrement(dimensionValues, Node::decrementEntries, false); + } + + /** + * Reset number of entries and memory size when all keys leave the cache, but don't reset hit/miss/eviction numbers. + * This is in line with the behavior of the existing API when caches are cleared. + */ + @Override + public void reset() { + resetHelper(statsRoot); + } + + private void resetHelper(Node current) { + current.resetSizeAndEntries(); + for (Node child : current.children.values()) { + resetHelper(child); + } + } + + @Override + public long count() { + // Include this here so caches don't have to create an entire CacheStats object to run count(). + return statsRoot.getEntries(); + } + + private void internalIncrement(List dimensionValues, Consumer adder, boolean createNodesIfAbsent) { + assert dimensionValues.size() == dimensionNames.size(); + // First try to increment without creating nodes + boolean didIncrement = internalIncrementHelper(dimensionValues, statsRoot, 0, adder, false); + // If we failed to increment, because nodes had to be created, obtain the lock and run again while creating nodes if needed + if (!didIncrement && createNodesIfAbsent) { + try { + lock.lock(); + internalIncrementHelper(dimensionValues, statsRoot, 0, adder, true); + } finally { + lock.unlock(); + } + } + } + + /** + * Use the incrementer function to increment/decrement a value in the stats for a set of dimensions. + * If createNodesIfAbsent is true, and there is no stats for this set of dimensions, create one. + * Returns true if the increment was applied, false if not. + */ + private boolean internalIncrementHelper( + List dimensionValues, + Node node, + int depth, // Pass in the depth to avoid having to slice the list for each node. + Consumer adder, + boolean createNodesIfAbsent + ) { + if (depth == dimensionValues.size()) { + // This is the leaf node we are trying to reach + adder.accept(node); + return true; + } + + Node child = node.getChild(dimensionValues.get(depth)); + if (child == null) { + if (createNodesIfAbsent) { + boolean createMapInChild = depth < dimensionValues.size() - 1; + child = node.createChild(dimensionValues.get(depth), createMapInChild); + } else { + return false; + } + } + if (internalIncrementHelper(dimensionValues, child, depth + 1, adder, createNodesIfAbsent)) { + // Function returns true if the next node down was incremented + adder.accept(node); + return true; + } + return false; + } + + /** + * Produce an immutable version of these stats. + */ + @Override + public ImmutableCacheStatsHolder getImmutableCacheStatsHolder() { + return new ImmutableCacheStatsHolder(statsRoot.snapshot(), dimensionNames); + } + + @Override + public void removeDimensions(List dimensionValues) { + assert dimensionValues.size() == dimensionNames.size() : "Must specify a value for every dimension when removing from StatsHolder"; + // As we are removing nodes from the tree, obtain the lock + lock.lock(); + try { + removeDimensionsHelper(dimensionValues, statsRoot, 0); + } finally { + lock.unlock(); + } + } + + // Returns a CacheStatsCounterSnapshot object for the stats to decrement if the removal happened, null otherwise. + private 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(); + } + 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; + } + + // pkg-private for testing + Node getStatsRoot() { + return statsRoot; + } + + static class Node { + private final String dimensionValue; + // Map from dimensionValue to the DimensionNode for that dimension value. + 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; + + // Used for leaf nodes to avoid allocating many unnecessary maps + private static final Map EMPTY_CHILDREN_MAP = new HashMap<>(); + + Node(String dimensionValue, boolean createChildrenMap) { + this.dimensionValue = dimensionValue; + if (createChildrenMap) { + this.children = new ConcurrentHashMap<>(); + } else { + this.children = EMPTY_CHILDREN_MAP; + } + this.stats = new CacheStats(); + } + + public String getDimensionValue() { + return dimensionValue; + } + + protected Map getChildren() { + // We can safely iterate over ConcurrentHashMap without worrying about thread issues. + return children; + } + + // Functions for modifying internal CacheStatsCounter without callers having to be aware of CacheStatsCounter + + void incrementHits() { + this.stats.incrementHits(); + } + + void incrementMisses() { + this.stats.incrementMisses(); + } + + void incrementEvictions() { + this.stats.incrementEvictions(); + } + + void incrementSizeInBytes(long amountBytes) { + this.stats.incrementSizeInBytes(amountBytes); + } + + void decrementSizeInBytes(long amountBytes) { + this.stats.decrementSizeInBytes(amountBytes); + } + + void incrementEntries() { + this.stats.incrementEntries(); + } + + void decrementEntries() { + this.stats.decrementEntries(); + } + + long getEntries() { + return this.stats.getEntries(); + } + + ImmutableCacheStats getImmutableStats() { + return this.stats.immutableSnapshot(); + } + + void decrementBySnapshot(ImmutableCacheStats snapshot) { + this.stats.subtract(snapshot); + } + + void resetSizeAndEntries() { + this.stats.resetSizeAndEntries(); + } + + Node getChild(String dimensionValue) { + return children.get(dimensionValue); + } + + Node createChild(String dimensionValue, boolean createMapInChild) { + return children.computeIfAbsent(dimensionValue, (key) -> new Node(dimensionValue, createMapInChild)); + } + + ImmutableCacheStatsHolder.Node snapshot() { + TreeMap snapshotChildren = null; + if (!children.isEmpty()) { + snapshotChildren = new TreeMap<>(); + for (Node child : children.values()) { + snapshotChildren.put(child.getDimensionValue(), child.snapshot()); + } + } + return new ImmutableCacheStatsHolder.Node(dimensionValue, snapshotChildren, getImmutableStats()); + } + } +} diff --git a/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java similarity index 76% rename from server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java rename to server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java index d397cb60c15bf..7743d03ce44ac 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/DummyCacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java @@ -17,15 +17,11 @@ * Will be removed once pluggable caches is no longer an experimental feature. * Returns all-zero stats when calling getImmutableCacheStatsHolder(). Does keep track of entries for use in ICache.count(). */ -public class DummyCacheStatsHolder implements CacheStatsHolderInterface { +public class NoopCacheStatsHolder implements CacheStatsHolder { private CounterMetric entries; - private CounterMetric sizeInBytes; - private final List dimensionNames; - public DummyCacheStatsHolder(List dimensionNames) { - this.dimensionNames = dimensionNames; + public NoopCacheStatsHolder() { this.entries = new CounterMetric(); - this.sizeInBytes = new CounterMetric(); } @Override @@ -38,14 +34,10 @@ public void incrementMisses(List dimensionValues) {} public void incrementEvictions(List dimensionValues) {} @Override - public void incrementSizeInBytes(List dimensionValues, long amountBytes) { - sizeInBytes.inc(amountBytes); - } + public void incrementSizeInBytes(List dimensionValues, long amountBytes) {} @Override - public void decrementSizeInBytes(List dimensionValues, long amountBytes) { - sizeInBytes.dec(amountBytes); - } + public void decrementSizeInBytes(List dimensionValues, long amountBytes) {} @Override public void incrementEntries(List dimensionValues) { @@ -60,7 +52,6 @@ public void decrementEntries(List dimensionValues) { @Override public void reset() { this.entries = new CounterMetric(); - this.sizeInBytes = new CounterMetric(); } @Override @@ -74,6 +65,6 @@ public void removeDimensions(List dimensionValues) {} @Override public ImmutableCacheStatsHolder getImmutableCacheStatsHolder() { ImmutableCacheStatsHolder.Node dummyNode = new ImmutableCacheStatsHolder.Node("", null, new ImmutableCacheStats(0, 0, 0, 0, 0)); - return new ImmutableCacheStatsHolder(dummyNode, dimensionNames); + return new ImmutableCacheStatsHolder(dummyNode, List.of()); } } diff --git a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java index 15cd9e4ba0a71..9d935b41b42a7 100644 --- a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java +++ b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java @@ -19,9 +19,9 @@ import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.settings.CacheSettings; import org.opensearch.common.cache.stats.CacheStatsHolder; -import org.opensearch.common.cache.stats.CacheStatsHolderInterface; -import org.opensearch.common.cache.stats.DummyCacheStatsHolder; +import org.opensearch.common.cache.stats.DefaultCacheStatsHolder; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; +import org.opensearch.common.cache.stats.NoopCacheStatsHolder; import org.opensearch.common.cache.store.builders.ICacheBuilder; import org.opensearch.common.cache.store.config.CacheConfig; import org.opensearch.common.cache.store.settings.OpenSearchOnHeapCacheSettings; @@ -49,7 +49,7 @@ public class OpenSearchOnHeapCache implements ICache, RemovalListener, V> { private final Cache, V> cache; - private final CacheStatsHolderInterface cacheStatsHolder; + private final CacheStatsHolder cacheStatsHolder; private final RemovalListener, V> removalListener; private final List dimensionNames; private final ToLongBiFunction, V> weigher; @@ -65,9 +65,9 @@ public OpenSearchOnHeapCache(Builder builder) { cache = cacheBuilder.build(); this.dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); if (FeatureFlags.PLUGGABLE_CACHE_SETTING.get(builder.getSettings())) { - this.cacheStatsHolder = new CacheStatsHolder(dimensionNames); + this.cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); } else { - this.cacheStatsHolder = new DummyCacheStatsHolder(dimensionNames); + this.cacheStatsHolder = new NoopCacheStatsHolder(); } this.removalListener = builder.getRemovalListener(); this.weigher = builder.getWeigher(); diff --git a/server/src/test/java/org/opensearch/common/cache/stats/CacheStatsHolderTests.java b/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java similarity index 85% rename from server/src/test/java/org/opensearch/common/cache/stats/CacheStatsHolderTests.java rename to server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java index 390cd4d601a4b..fe12673bb9f6a 100644 --- a/server/src/test/java/org/opensearch/common/cache/stats/CacheStatsHolderTests.java +++ b/server/src/test/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolderTests.java @@ -21,18 +21,23 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; -public class CacheStatsHolderTests extends OpenSearchTestCase { +public class DefaultCacheStatsHolderTests extends OpenSearchTestCase { public void testAddAndGet() throws Exception { List dimensionNames = List.of("dim1", "dim2", "dim3", "dim4"); - CacheStatsHolder cacheStatsHolder = new CacheStatsHolder(dimensionNames); - Map> usedDimensionValues = CacheStatsHolderTests.getUsedDimensionValues(cacheStatsHolder, 10); - Map, CacheStats> expected = CacheStatsHolderTests.populateStats(cacheStatsHolder, usedDimensionValues, 1000, 10); + DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); + Map> usedDimensionValues = DefaultCacheStatsHolderTests.getUsedDimensionValues(cacheStatsHolder, 10); + Map, CacheStats> expected = DefaultCacheStatsHolderTests.populateStats( + cacheStatsHolder, + usedDimensionValues, + 1000, + 10 + ); // test the value in the map is as expected for each distinct combination of values for (List dimensionValues : expected.keySet()) { CacheStats expectedCounter = expected.get(dimensionValues); - ImmutableCacheStats actualStatsHolder = CacheStatsHolderTests.getNode(dimensionValues, cacheStatsHolder.getStatsRoot()) + ImmutableCacheStats actualStatsHolder = DefaultCacheStatsHolderTests.getNode(dimensionValues, cacheStatsHolder.getStatsRoot()) .getImmutableStats(); ImmutableCacheStats actualCacheStats = getNode(dimensionValues, cacheStatsHolder.getStatsRoot()).getImmutableStats(); @@ -53,7 +58,7 @@ public void testAddAndGet() throws Exception { public void testReset() throws Exception { List dimensionNames = List.of("dim1", "dim2"); - CacheStatsHolder cacheStatsHolder = new CacheStatsHolder(dimensionNames); + DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10); Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 100, 10); @@ -64,7 +69,7 @@ public void testReset() throws Exception { originalCounter.sizeInBytes = new CounterMetric(); originalCounter.entries = new CounterMetric(); - CacheStatsHolder.Node node = getNode(dimensionValues, cacheStatsHolder.getStatsRoot()); + DefaultCacheStatsHolder.Node node = getNode(dimensionValues, cacheStatsHolder.getStatsRoot()); ImmutableCacheStats actual = node.getImmutableStats(); assertEquals(originalCounter.immutableSnapshot(), actual); } @@ -72,7 +77,7 @@ public void testReset() throws Exception { public void testDropStatsForDimensions() throws Exception { List dimensionNames = List.of("dim1", "dim2"); - CacheStatsHolder cacheStatsHolder = new CacheStatsHolder(dimensionNames); + DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); // Create stats for the following dimension sets List> populatedStats = List.of(List.of("A1", "B1"), List.of("A2", "B2"), List.of("A2", "B3")); @@ -108,7 +113,7 @@ public void testDropStatsForDimensions() throws Exception { public void testCount() throws Exception { List dimensionNames = List.of("dim1", "dim2"); - CacheStatsHolder cacheStatsHolder = new CacheStatsHolder(dimensionNames); + DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); Map> usedDimensionValues = getUsedDimensionValues(cacheStatsHolder, 10); Map, CacheStats> expected = populateStats(cacheStatsHolder, usedDimensionValues, 100, 10); @@ -121,7 +126,7 @@ public void testCount() throws Exception { public void testConcurrentRemoval() throws Exception { List dimensionNames = List.of("dim1", "dim2"); - CacheStatsHolder cacheStatsHolder = new CacheStatsHolder(dimensionNames); + DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); // Create stats for the following dimension sets List> populatedStats = List.of(List.of("A1", "B1"), List.of("A2", "B2"), List.of("A2", "B3")); @@ -169,8 +174,8 @@ public void testConcurrentRemoval() throws Exception { * Returns the node found by following these dimension values down from the root node. * Returns null if no such node exists. */ - static CacheStatsHolder.Node getNode(List dimensionValues, CacheStatsHolder.Node root) { - CacheStatsHolder.Node current = root; + 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) { @@ -181,7 +186,7 @@ static CacheStatsHolder.Node getNode(List dimensionValues, CacheStatsHol } static Map, CacheStats> populateStats( - CacheStatsHolder cacheStatsHolder, + DefaultCacheStatsHolder cacheStatsHolder, Map> usedDimensionValues, int numDistinctValuePairs, int numRepetitionsPerValue @@ -211,7 +216,7 @@ static Map, CacheStats> populateStats( expected.get(dimensions).evictions.inc(statsToInc.getEvictions()); expected.get(dimensions).sizeInBytes.inc(statsToInc.getSizeInBytes()); expected.get(dimensions).entries.inc(statsToInc.getEntries()); - CacheStatsHolderTests.populateStatsHolderFromStatsValueMap(cacheStatsHolder, Map.of(dimensions, statsToInc)); + DefaultCacheStatsHolderTests.populateStatsHolderFromStatsValueMap(cacheStatsHolder, Map.of(dimensions, statsToInc)); } countDownLatch.countDown(); }); @@ -240,7 +245,7 @@ private static List getRandomDimList( return result; } - static Map> getUsedDimensionValues(CacheStatsHolder cacheStatsHolder, int numValuesPerDim) { + static Map> getUsedDimensionValues(DefaultCacheStatsHolder cacheStatsHolder, int numValuesPerDim) { Map> usedDimensionValues = new HashMap<>(); for (int i = 0; i < cacheStatsHolder.getDimensionNames().size(); i++) { List values = new ArrayList<>(); @@ -252,20 +257,20 @@ static Map> getUsedDimensionValues(CacheStatsHolder cacheSt return usedDimensionValues; } - private void assertSumOfChildrenStats(CacheStatsHolder.Node current) { + private void assertSumOfChildrenStats(DefaultCacheStatsHolder.Node current) { if (!current.children.isEmpty()) { CacheStats expectedTotal = new CacheStats(); - for (CacheStatsHolder.Node child : current.children.values()) { + for (DefaultCacheStatsHolder.Node child : current.children.values()) { expectedTotal.add(child.getImmutableStats()); } assertEquals(expectedTotal.immutableSnapshot(), current.getImmutableStats()); - for (CacheStatsHolder.Node child : current.children.values()) { + for (DefaultCacheStatsHolder.Node child : current.children.values()) { assertSumOfChildrenStats(child); } } } - static void populateStatsHolderFromStatsValueMap(CacheStatsHolder cacheStatsHolder, Map, CacheStats> statsMap) { + static void populateStatsHolderFromStatsValueMap(DefaultCacheStatsHolder cacheStatsHolder, Map, CacheStats> statsMap) { for (Map.Entry, CacheStats> entry : statsMap.entrySet()) { CacheStats stats = entry.getValue(); List dims = entry.getKey(); diff --git a/server/src/test/java/org/opensearch/common/cache/stats/ImmutableCacheStatsHolderTests.java b/server/src/test/java/org/opensearch/common/cache/stats/ImmutableCacheStatsHolderTests.java index 933b8abd6e392..5a4511fa654dd 100644 --- a/server/src/test/java/org/opensearch/common/cache/stats/ImmutableCacheStatsHolderTests.java +++ b/server/src/test/java/org/opensearch/common/cache/stats/ImmutableCacheStatsHolderTests.java @@ -17,17 +17,24 @@ public class ImmutableCacheStatsHolderTests extends OpenSearchTestCase { public void testGet() throws Exception { List dimensionNames = List.of("dim1", "dim2", "dim3", "dim4"); - CacheStatsHolder cacheStatsHolder = new CacheStatsHolder(dimensionNames); - Map> usedDimensionValues = CacheStatsHolderTests.getUsedDimensionValues(cacheStatsHolder, 10); - Map, CacheStats> expected = CacheStatsHolderTests.populateStats(cacheStatsHolder, usedDimensionValues, 1000, 10); + DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); + Map> usedDimensionValues = DefaultCacheStatsHolderTests.getUsedDimensionValues(cacheStatsHolder, 10); + Map, CacheStats> expected = DefaultCacheStatsHolderTests.populateStats( + cacheStatsHolder, + usedDimensionValues, + 1000, + 10 + ); ImmutableCacheStatsHolder stats = cacheStatsHolder.getImmutableCacheStatsHolder(); // test the value in the map is as expected for each distinct combination of values for (List dimensionValues : expected.keySet()) { CacheStats expectedCounter = expected.get(dimensionValues); - ImmutableCacheStats actualCacheStatsHolder = CacheStatsHolderTests.getNode(dimensionValues, cacheStatsHolder.getStatsRoot()) - .getImmutableStats(); + ImmutableCacheStats actualCacheStatsHolder = DefaultCacheStatsHolderTests.getNode( + dimensionValues, + cacheStatsHolder.getStatsRoot() + ).getImmutableStats(); ImmutableCacheStats actualImmutableCacheStatsHolder = getNode(dimensionValues, stats.getStatsRoot()).getStats(); assertEquals(expectedCounter.immutableSnapshot(), actualCacheStatsHolder); @@ -52,9 +59,9 @@ public void testGet() throws Exception { public void testEmptyDimsList() throws Exception { // If the dimension list is empty, the tree should have only the root node containing the total stats. - CacheStatsHolder cacheStatsHolder = new CacheStatsHolder(List.of()); - Map> usedDimensionValues = CacheStatsHolderTests.getUsedDimensionValues(cacheStatsHolder, 100); - CacheStatsHolderTests.populateStats(cacheStatsHolder, usedDimensionValues, 10, 100); + DefaultCacheStatsHolder cacheStatsHolder = new DefaultCacheStatsHolder(List.of()); + Map> usedDimensionValues = DefaultCacheStatsHolderTests.getUsedDimensionValues(cacheStatsHolder, 100); + DefaultCacheStatsHolderTests.populateStats(cacheStatsHolder, usedDimensionValues, 10, 100); ImmutableCacheStatsHolder stats = cacheStatsHolder.getImmutableCacheStatsHolder(); ImmutableCacheStatsHolder.Node statsRoot = stats.getStatsRoot(); From bafb9184e2474b7e721aaf694d1f79c6acab6ca6 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 18 Apr 2024 12:38:47 -0700 Subject: [PATCH 07/15] Reworded comments Signed-off-by: Peter Alfonsi --- .../java/org/opensearch/common/cache/stats/CacheStatsHolder.java | 1 - .../org/opensearch/common/cache/stats/NoopCacheStatsHolder.java | 1 - 2 files changed, 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java index cdc910117b862..a1cfb8d806af3 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/CacheStatsHolder.java @@ -12,7 +12,6 @@ /** * An interface extended by DefaultCacheStatsHolder and NoopCacheStatsHolder. - * Can be removed once the pluggable caches feature is no longer experimental. */ public interface CacheStatsHolder { void incrementHits(List dimensionValues); diff --git a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java index 7743d03ce44ac..c98f81d683b36 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java @@ -14,7 +14,6 @@ /** * A dummy version of CacheStatsHolder, which cache implementations use when FeatureFlags.PLUGGABLE_CACHES is false. - * Will be removed once pluggable caches is no longer an experimental feature. * Returns all-zero stats when calling getImmutableCacheStatsHolder(). Does keep track of entries for use in ICache.count(). */ public class NoopCacheStatsHolder implements CacheStatsHolder { From 8b63a7289de46b20f8aa736440f7a15b2cd21c2b Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 18 Apr 2024 13:02:58 -0700 Subject: [PATCH 08/15] Removed count() from NoopCacheStatsHolder Signed-off-by: Peter Alfonsi --- .../cache/stats/NoopCacheStatsHolder.java | 23 +++++-------------- .../cache/store/OpenSearchOnHeapCache.java | 20 ++++++++++++---- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java index c98f81d683b36..ea050d306b5a7 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java @@ -8,20 +8,15 @@ package org.opensearch.common.cache.stats; -import org.opensearch.common.metrics.CounterMetric; - import java.util.List; /** * A dummy version of CacheStatsHolder, which cache implementations use when FeatureFlags.PLUGGABLE_CACHES is false. - * Returns all-zero stats when calling getImmutableCacheStatsHolder(). Does keep track of entries for use in ICache.count(). + * Returns all-zero stats when calling getImmutableCacheStatsHolder(). Always returns 0 for count(). */ public class NoopCacheStatsHolder implements CacheStatsHolder { - private CounterMetric entries; - public NoopCacheStatsHolder() { - this.entries = new CounterMetric(); - } + public NoopCacheStatsHolder() {} @Override public void incrementHits(List dimensionValues) {} @@ -39,23 +34,17 @@ public void incrementSizeInBytes(List dimensionValues, long amountBytes) public void decrementSizeInBytes(List dimensionValues, long amountBytes) {} @Override - public void incrementEntries(List dimensionValues) { - entries.inc(); - } + public void incrementEntries(List dimensionValues) {} @Override - public void decrementEntries(List dimensionValues) { - entries.dec(); - } + public void decrementEntries(List dimensionValues) {} @Override - public void reset() { - this.entries = new CounterMetric(); - } + public void reset() {} @Override public long count() { - return entries.count(); + return 0; } @Override diff --git a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java index 9d935b41b42a7..e03a9ac8693f3 100644 --- a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java +++ b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java @@ -54,6 +54,8 @@ public class OpenSearchOnHeapCache implements ICache, RemovalListene private final List dimensionNames; private final ToLongBiFunction, V> weigher; + private final Settings settings; + public OpenSearchOnHeapCache(Builder builder) { CacheBuilder, V> cacheBuilder = CacheBuilder., V>builder() .setMaximumWeight(builder.getMaxWeightInBytes()) @@ -64,13 +66,14 @@ public OpenSearchOnHeapCache(Builder builder) { } cache = cacheBuilder.build(); this.dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); - if (FeatureFlags.PLUGGABLE_CACHE_SETTING.get(builder.getSettings())) { - this.cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); - } else { + if (useNoopStats(builder.getSettings())) { this.cacheStatsHolder = new NoopCacheStatsHolder(); + } else { + this.cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); } this.removalListener = builder.getRemovalListener(); this.weigher = builder.getWeigher(); + this.settings = builder.getSettings(); } @Override @@ -127,7 +130,11 @@ public Iterable> keys() { @Override public long count() { - return cacheStatsHolder.count(); + if (useNoopStats(settings)) { + return cache.count(); + } else { + return cacheStatsHolder.count(); + } } @Override @@ -158,6 +165,11 @@ public void onRemoval(RemovalNotification, V> notification) { } } + private boolean useNoopStats(Settings settings) { + // Use noop stats when pluggable caching is off + return !FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings); + } + /** * Factory to create OpenSearchOnheap cache. */ From c1d7bf9a209588e6aa0fa7c7310f6a34649f5ebd Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 23 Apr 2024 13:20:41 -0700 Subject: [PATCH 09/15] Addressed Ankit's comments Signed-off-by: Peter Alfonsi --- .../cache/stats/NoopCacheStatsHolder.java | 8 ++++- .../cache/store/OpenSearchOnHeapCache.java | 29 +++++++++++-------- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java index ea050d306b5a7..47a4b54ba5f93 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java @@ -13,10 +13,16 @@ /** * A dummy version of CacheStatsHolder, which cache implementations use when FeatureFlags.PLUGGABLE_CACHES is false. * Returns all-zero stats when calling getImmutableCacheStatsHolder(). Always returns 0 for count(). + * A singleton instance is used for memory purposes. */ public class NoopCacheStatsHolder implements CacheStatsHolder { + private static final NoopCacheStatsHolder singletonInstance = new NoopCacheStatsHolder(); - public NoopCacheStatsHolder() {} + private NoopCacheStatsHolder() {} + + public static NoopCacheStatsHolder getInstance() { + return singletonInstance; + } @Override public void incrementHits(List dimensionValues) {} diff --git a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java index e03a9ac8693f3..13c98042d4950 100644 --- a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java +++ b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java @@ -53,8 +53,7 @@ public class OpenSearchOnHeapCache implements ICache, RemovalListene private final RemovalListener, V> removalListener; private final List dimensionNames; private final ToLongBiFunction, V> weigher; - - private final Settings settings; + private final boolean useNoopStats; public OpenSearchOnHeapCache(Builder builder) { CacheBuilder, V> cacheBuilder = CacheBuilder., V>builder() @@ -66,14 +65,14 @@ public OpenSearchOnHeapCache(Builder builder) { } cache = cacheBuilder.build(); this.dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); - if (useNoopStats(builder.getSettings())) { - this.cacheStatsHolder = new NoopCacheStatsHolder(); + this.useNoopStats = builder.useNoopStats; + if (useNoopStats) { + this.cacheStatsHolder = NoopCacheStatsHolder.getInstance(); } else { this.cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames); } this.removalListener = builder.getRemovalListener(); this.weigher = builder.getWeigher(); - this.settings = builder.getSettings(); } @Override @@ -130,7 +129,7 @@ public Iterable> keys() { @Override public long count() { - if (useNoopStats(settings)) { + if (useNoopStats) { return cache.count(); } else { return cacheStatsHolder.count(); @@ -165,11 +164,6 @@ public void onRemoval(RemovalNotification, V> notification) { } } - private boolean useNoopStats(Settings settings) { - // Use noop stats when pluggable caching is off - return !FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings); - } - /** * Factory to create OpenSearchOnheap cache. */ @@ -182,10 +176,10 @@ public ICache create(CacheConfig config, CacheType cacheType, Map> settingList = OpenSearchOnHeapCacheSettings.getSettingListForCacheType(cacheType); Settings settings = config.getSettings(); ICacheBuilder builder = new Builder().setDimensionNames(config.getDimensionNames()) + .setUseNoopStats(useNoopStats(config.getSettings())) .setMaximumWeightInBytes(((ByteSizeValue) settingList.get(MAXIMUM_SIZE_IN_BYTES_KEY).get(settings)).getBytes()) .setExpireAfterAccess(((TimeValue) settingList.get(EXPIRE_AFTER_ACCESS_KEY).get(settings))) .setWeigher(config.getWeigher()) - .setSettings(config.getSettings()) .setRemovalListener(config.getRemovalListener()); Setting cacheSettingForCacheType = CacheSettings.CACHE_TYPE_STORE_NAME.getConcreteSettingForNamespace( cacheType.getSettingPrefix() @@ -203,6 +197,11 @@ public ICache create(CacheConfig config, CacheType cacheType, public String getCacheName() { return NAME; } + + private boolean useNoopStats(Settings settings) { + // Use noop stats when pluggable caching is off + return !FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings); + } } /** @@ -212,12 +211,18 @@ public String getCacheName() { */ public static class Builder extends ICacheBuilder { private List dimensionNames; + private boolean useNoopStats; public Builder setDimensionNames(List dimensionNames) { this.dimensionNames = dimensionNames; return this; } + public Builder setUseNoopStats(boolean useNoopStats) { + this.useNoopStats = useNoopStats; + return this; + } + @Override public ICache build() { return new OpenSearchOnHeapCache(this); From 2abb89dd72a12a207bdf9efdace8cba726b1c088 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 23 Apr 2024 16:26:33 -0700 Subject: [PATCH 10/15] Addressed comment Signed-off-by: Peter Alfonsi --- .../common/cache/stats/NoopCacheStatsHolder.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java index 47a4b54ba5f93..b7debbd8a8eab 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java @@ -17,6 +17,11 @@ */ public class NoopCacheStatsHolder implements CacheStatsHolder { private static final NoopCacheStatsHolder singletonInstance = new NoopCacheStatsHolder(); + private static final ImmutableCacheStatsHolder immutableCacheStatsHolder; + static { + ImmutableCacheStatsHolder.Node dummyNode = new ImmutableCacheStatsHolder.Node("", null, new ImmutableCacheStats(0, 0, 0, 0, 0)); + immutableCacheStatsHolder = new ImmutableCacheStatsHolder(dummyNode, List.of()); + } private NoopCacheStatsHolder() {} @@ -58,7 +63,6 @@ public void removeDimensions(List dimensionValues) {} @Override public ImmutableCacheStatsHolder getImmutableCacheStatsHolder() { - ImmutableCacheStatsHolder.Node dummyNode = new ImmutableCacheStatsHolder.Node("", null, new ImmutableCacheStats(0, 0, 0, 0, 0)); - return new ImmutableCacheStatsHolder(dummyNode, List.of()); + return immutableCacheStatsHolder; } } From c708bc70a18af9bb17b82b9d370fb953798c3146 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 25 Apr 2024 08:45:01 -0700 Subject: [PATCH 11/15] Addressed Michael's comments Signed-off-by: Peter Alfonsi --- .../cache/store/OpenSearchOnHeapCache.java | 23 ++++--------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java index 13c98042d4950..35c951e240a3a 100644 --- a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java +++ b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java @@ -53,7 +53,6 @@ public class OpenSearchOnHeapCache implements ICache, RemovalListene private final RemovalListener, V> removalListener; private final List dimensionNames; private final ToLongBiFunction, V> weigher; - private final boolean useNoopStats; public OpenSearchOnHeapCache(Builder builder) { CacheBuilder, V> cacheBuilder = CacheBuilder., V>builder() @@ -65,7 +64,8 @@ public OpenSearchOnHeapCache(Builder builder) { } cache = cacheBuilder.build(); this.dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); - this.useNoopStats = builder.useNoopStats; + // Use noop stats when pluggable caching is off + boolean useNoopStats = !FeatureFlags.PLUGGABLE_CACHE_SETTING.get(builder.getSettings()); if (useNoopStats) { this.cacheStatsHolder = NoopCacheStatsHolder.getInstance(); } else { @@ -129,11 +129,7 @@ public Iterable> keys() { @Override public long count() { - if (useNoopStats) { - return cache.count(); - } else { - return cacheStatsHolder.count(); - } + return cache.count(); } @Override @@ -176,7 +172,7 @@ public ICache create(CacheConfig config, CacheType cacheType, Map> settingList = OpenSearchOnHeapCacheSettings.getSettingListForCacheType(cacheType); Settings settings = config.getSettings(); ICacheBuilder builder = new Builder().setDimensionNames(config.getDimensionNames()) - .setUseNoopStats(useNoopStats(config.getSettings())) + .setSettings(config.getSettings()) .setMaximumWeightInBytes(((ByteSizeValue) settingList.get(MAXIMUM_SIZE_IN_BYTES_KEY).get(settings)).getBytes()) .setExpireAfterAccess(((TimeValue) settingList.get(EXPIRE_AFTER_ACCESS_KEY).get(settings))) .setWeigher(config.getWeigher()) @@ -197,11 +193,6 @@ public ICache create(CacheConfig config, CacheType cacheType, public String getCacheName() { return NAME; } - - private boolean useNoopStats(Settings settings) { - // Use noop stats when pluggable caching is off - return !FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings); - } } /** @@ -211,18 +202,12 @@ private boolean useNoopStats(Settings settings) { */ public static class Builder extends ICacheBuilder { private List dimensionNames; - private boolean useNoopStats; public Builder setDimensionNames(List dimensionNames) { this.dimensionNames = dimensionNames; return this; } - public Builder setUseNoopStats(boolean useNoopStats) { - this.useNoopStats = useNoopStats; - return this; - } - @Override public ICache build() { return new OpenSearchOnHeapCache(this); From 239b711314d3b0414de2f6553fdc7d4ad495d9cd Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 26 Apr 2024 12:23:36 -0700 Subject: [PATCH 12/15] rerunning assemble Signed-off-by: Peter Alfonsi From 87da3ef068213b57834f80ee7a46731c23a1ca9a Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 26 Apr 2024 13:57:09 -0700 Subject: [PATCH 13/15] rerun Signed-off-by: Peter Alfonsi From 9b0935f0fae611ac2c21da8644e6c677c3499d25 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 26 Apr 2024 23:59:02 -0700 Subject: [PATCH 14/15] rerun Signed-off-by: Peter Alfonsi From f935dde09ff53bf0e3f3fe8e99b25d25e0094676 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Sat, 27 Apr 2024 11:06:56 -0700 Subject: [PATCH 15/15] rerun Signed-off-by: Peter Alfonsi