From 83a44f5a9ffcf401b03513e635774ff26e4daabf Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 13 Nov 2024 11:50:37 -0800 Subject: [PATCH 1/4] Fix cache size setting Signed-off-by: Peter Alfonsi --- .../tier/TieredSpilloverCacheSettings.java | 4 + .../cache/common/tier/MockDiskCache.java | 4 + .../tier/TieredSpilloverCacheTests.java | 140 +++++++++++++++++- .../cache/EhcacheDiskCacheSettings.java | 1 + .../cache/store/disk/EhcacheDiskCache.java | 5 + .../store/disk/EhCacheDiskCacheTests.java | 62 ++++++++ .../common/cache/service/CacheService.java | 13 +- .../cache/store/OpenSearchOnHeapCache.java | 16 +- .../OpenSearchOnHeapCacheSettings.java | 1 + .../indices/IndicesRequestCache.java | 70 +++++---- .../store/OpenSearchOnHeapCacheTests.java | 75 +++++++--- .../indices/IndicesRequestCacheTests.java | 38 +++++ 12 files changed, 371 insertions(+), 58 deletions(-) diff --git a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java index 122d00af3bd1e..36a2aba28f879 100644 --- a/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java +++ b/modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheSettings.java @@ -85,6 +85,9 @@ public class TieredSpilloverCacheSettings { /** * Setting which defines the onHeap cache size to be used within tiered cache. + * This setting overrides size settings from the heap tier implementation. + * For example, if OpenSearchOnHeapCache is the heap tier in the request cache, and + * indices.requests.cache.opensearch_onheap.size is set, that value will be ignored in favor of this setting. * * Pattern: {cache_type}.tiered_spillover.onheap.store.size * Example: indices.request.cache.tiered_spillover.onheap.store.size @@ -96,6 +99,7 @@ public class TieredSpilloverCacheSettings { /** * Setting which defines the disk cache size to be used within tiered cache. + * Similarly, this setting overrides the size setting from the disk tier implementation. */ public static final Setting.AffixSetting TIERED_SPILLOVER_DISK_STORE_SIZE = Setting.suffixKeySetting( TieredSpilloverCache.TieredSpilloverCacheFactory.TIERED_SPILLOVER_CACHE_NAME + ".disk.store.size", diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java index fcddd489a27aa..f1d7e2c113466 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/MockDiskCache.java @@ -128,6 +128,10 @@ public void close() { } + long getMaxSize() { + return maxSize; + } + public static class MockDiskCacheFactory implements Factory { public static final String NAME = "mockDiskCache"; diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java index 1215a2130ac2d..c1e049750ba8d 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java @@ -58,6 +58,7 @@ import static org.opensearch.cache.common.tier.TieredSpilloverCache.ZERO_SEGMENT_COUNT_EXCEPTION_MESSAGE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.DISK_CACHE_ENABLED_SETTING_MAP; +import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.MIN_DISK_CACHE_SIZE_IN_BYTES; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS; import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP; @@ -2112,6 +2113,139 @@ public void testTieredCacheDefaultSegmentCount() { assertTrue(VALID_SEGMENT_COUNT_VALUES.contains(tieredSpilloverCache.getNumberOfSegments())); } + public void testSegmentSizesWhenUsingFactory() { + // The TSC's tier size settings, TIERED_SPILLOVER_ONHEAP_STORE_SIZE and TIERED_SPILLOVER_DISK_STORE_SIZE, + // should always be respected, overriding the individual implementation's size settings if present + long expectedHeapSize = 256L * between(10, 20); + long expectedDiskSize = MIN_DISK_CACHE_SIZE_IN_BYTES + 256L * between(30, 40); + long heapSizeFromImplSetting = 50; + int diskSizeFromImplSetting = 50; + int numSegments = getNumberOfSegments(); + + int keyValueSize = 1; + MockCacheRemovalListener removalListener = new MockCacheRemovalListener<>(); + Settings settings = Settings.builder() + .put( + CacheSettings.getConcreteStoreNameSettingForCacheType(CacheType.INDICES_REQUEST_CACHE).getKey(), + TieredSpilloverCache.TieredSpilloverCacheFactory.TIERED_SPILLOVER_CACHE_NAME + ) + .put( + TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_NAME.getConcreteSettingForNamespace( + CacheType.INDICES_REQUEST_CACHE.getSettingPrefix() + ).getKey(), + OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory.NAME + ) + .put( + TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_NAME.getConcreteSettingForNamespace( + CacheType.INDICES_REQUEST_CACHE.getSettingPrefix() + ).getKey(), + MockDiskCache.MockDiskCacheFactory.NAME + ) + // These two size settings should be honored + .put( + TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE.getConcreteSettingForNamespace( + CacheType.INDICES_REQUEST_CACHE.getSettingPrefix() + ).getKey(), + expectedHeapSize + "b" + ) + .put( + TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE.getConcreteSettingForNamespace( + CacheType.INDICES_REQUEST_CACHE.getSettingPrefix() + ).getKey(), + expectedDiskSize + ) + // The size setting from the OpenSearchOnHeap implementation should not be honored + .put( + OpenSearchOnHeapCacheSettings.MAXIMUM_SIZE_IN_BYTES.getConcreteSettingForNamespace( + CacheType.INDICES_REQUEST_CACHE.getSettingPrefix() + ).getKey(), + heapSizeFromImplSetting + "b" + ) + .put(FeatureFlags.PLUGGABLE_CACHE, "true") + .put( + TIERED_SPILLOVER_SEGMENTS.getConcreteSettingForNamespace(CacheType.INDICES_REQUEST_CACHE.getSettingPrefix()).getKey(), + numSegments + ) + .build(); + String storagePath = getStoragePath(settings); + + TieredSpilloverCache tieredSpilloverCache = (TieredSpilloverCache< + String, + String>) new TieredSpilloverCache.TieredSpilloverCacheFactory().create( + new CacheConfig.Builder().setKeyType(String.class) + .setKeyType(String.class) + .setWeigher((k, v) -> keyValueSize) + .setRemovalListener(removalListener) + .setKeySerializer(new StringSerializer()) + .setValueSerializer(new StringSerializer()) + .setSettings(settings) + .setDimensionNames(dimensionNames) + .setCachedResultParser(s -> new CachedQueryResult.PolicyValues(20_000_000L)) // Values will always appear to have taken + // 20_000_000 ns = 20 ms to compute + .setClusterSettings(clusterSettings) + .setStoragePath(storagePath) + .build(), + CacheType.INDICES_REQUEST_CACHE, + Map.of( + OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory.NAME, + new OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory(), + MockDiskCache.MockDiskCacheFactory.NAME, + // The size value passed in here acts as the "implementation setting" for the disk tier, and should also be ignored + new MockDiskCache.MockDiskCacheFactory(0, diskSizeFromImplSetting, false, keyValueSize) + ) + ); + checkSegmentSizes(tieredSpilloverCache, expectedHeapSize, expectedDiskSize); + } + + public void testSegmentSizesWhenNotUsingFactory() { + long expectedHeapSize = 256L * between(10, 20); + long expectedDiskSize = MIN_DISK_CACHE_SIZE_IN_BYTES + 256L * between(30, 40); + int heapSizeFromImplSetting = 50; + int diskSizeFromImplSetting = 50; + + Settings settings = Settings.builder() + .put( + CacheSettings.getConcreteStoreNameSettingForCacheType(CacheType.INDICES_REQUEST_CACHE).getKey(), + TieredSpilloverCache.TieredSpilloverCacheFactory.TIERED_SPILLOVER_CACHE_NAME + ) + .put(FeatureFlags.PLUGGABLE_CACHE, "true") + // The size setting from the OpenSearchOnHeapCache implementation should not be honored + .put( + OpenSearchOnHeapCacheSettings.MAXIMUM_SIZE_IN_BYTES.getConcreteSettingForNamespace( + CacheType.INDICES_REQUEST_CACHE.getSettingPrefix() + ).getKey(), + heapSizeFromImplSetting + "b" + ) + .build(); + + int keyValueSize = 1; + MockCacheRemovalListener removalListener = new MockCacheRemovalListener<>(); + int numSegments = getNumberOfSegments(); + CacheConfig cacheConfig = getCacheConfig(1, settings, removalListener, numSegments); + TieredSpilloverCache tieredSpilloverCache = getTieredSpilloverCache( + new OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory(), + new MockDiskCache.MockDiskCacheFactory(0, diskSizeFromImplSetting, true, keyValueSize), + cacheConfig, + null, + removalListener, + numSegments, + expectedHeapSize, + expectedDiskSize + ); + checkSegmentSizes(tieredSpilloverCache, expectedHeapSize, expectedDiskSize); + } + + private void checkSegmentSizes(TieredSpilloverCache cache, long expectedHeapSize, long expectedDiskSize) { + OpenSearchOnHeapCache segmentHeapCache = (OpenSearchOnHeapCache< + String, + String>) cache.tieredSpilloverCacheSegments[0].getOnHeapCache(); + assertEquals(expectedHeapSize / cache.getNumberOfSegments(), segmentHeapCache.getMaximumWeight()); + + MockDiskCache segmentDiskCache = (MockDiskCache) cache.tieredSpilloverCacheSegments[0] + .getDiskCache(); + assertEquals(expectedDiskSize / cache.getNumberOfSegments(), segmentDiskCache.getMaxSize()); + } + private List getMockDimensions() { List dims = new ArrayList<>(); for (String dimensionName : dimensionNames) { @@ -2401,9 +2535,9 @@ private void verifyComputeIfAbsentThrowsException( MockCacheRemovalListener removalListener = new MockCacheRemovalListener<>(); Settings settings = Settings.builder() .put( - OpenSearchOnHeapCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) - .get(MAXIMUM_SIZE_IN_BYTES_KEY) - .getKey(), + TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE.getConcreteSettingForNamespace( + CacheType.INDICES_REQUEST_CACHE.getSettingPrefix() + ).getKey(), onHeapCacheSize * keyValueSize + "b" ) .build(); diff --git a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/EhcacheDiskCacheSettings.java b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/EhcacheDiskCacheSettings.java index cbc104f2d0b00..e4c9dd1e96c3c 100644 --- a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/EhcacheDiskCacheSettings.java +++ b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/EhcacheDiskCacheSettings.java @@ -101,6 +101,7 @@ public class EhcacheDiskCacheSettings { /** * Disk cache max size setting. + * If this cache is used as a tier in a TieredSpilloverCache, this setting is ignored. */ public static final Setting.AffixSetting DISK_CACHE_MAX_SIZE_IN_BYTES_SETTING = Setting.suffixKeySetting( EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + ".max_size_in_bytes", 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 0fa0f8162bb98..28de573d87a6c 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 @@ -680,6 +680,11 @@ private V deserializeValue(ByteArrayWrapper binary) { return valueSerializer.deserialize(binary.value); } + // Pkg-private for testing. + long getMaxWeightInBytes() { + return maxWeightInBytes; + } + /** * Factory to create an ehcache disk cache. */ 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 a0d0aa4ec4914..cfa0940101ad1 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 @@ -20,11 +20,13 @@ import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.serializer.BytesReferenceSerializer; import org.opensearch.common.cache.serializer.Serializer; +import org.opensearch.common.cache.settings.CacheSettings; import org.opensearch.common.cache.stats.ImmutableCacheStats; import org.opensearch.common.cache.store.config.CacheConfig; 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.common.util.io.IOUtils; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; @@ -51,6 +53,7 @@ import java.util.function.ToLongBiFunction; import org.ehcache.PersistentCacheManager; +import org.ehcache.impl.serialization.StringSerializer; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_LISTENER_MODE_SYNC_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_MAX_SIZE_IN_BYTES_KEY; @@ -1201,6 +1204,65 @@ public void testEhcacheCloseWithDestroyCacheMethodThrowingException() throws Exc ehcacheDiskCache.close(); } + public void testWithCacheConfigSizeSettings() throws Exception { + // The cache should get its size from the config if present, and otherwise should get it from the setting. + long maxSizeFromSetting = between(MINIMUM_MAX_SIZE_IN_BYTES + 1000, MINIMUM_MAX_SIZE_IN_BYTES + 2000); + long maxSizeFromConfig = between(MINIMUM_MAX_SIZE_IN_BYTES + 3000, MINIMUM_MAX_SIZE_IN_BYTES + 4000); + + EhcacheDiskCache cache = setupMaxSizeTest(maxSizeFromSetting, maxSizeFromConfig, false); + assertEquals(maxSizeFromSetting, cache.getMaxWeightInBytes()); + + cache = setupMaxSizeTest(maxSizeFromSetting, maxSizeFromConfig, true); + assertEquals(maxSizeFromConfig, cache.getMaxWeightInBytes()); + } + + // Modified from OpenSearchOnHeapCacheTests. Can't reuse, as we can't add a dependency on the server.test module. + private EhcacheDiskCache setupMaxSizeTest(long maxSizeFromSetting, long maxSizeFromConfig, boolean putSizeInConfig) + throws Exception { + MockRemovalListener listener = new MockRemovalListener<>(); + try (NodeEnvironment env = newNodeEnvironment(Settings.builder().build())) { + Settings settings = Settings.builder() + .put(FeatureFlags.PLUGGABLE_CACHE, true) + .put( + CacheSettings.getConcreteStoreNameSettingForCacheType(CacheType.INDICES_REQUEST_CACHE).getKey(), + EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + ) + .put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_MAX_SIZE_IN_BYTES_KEY) + .getKey(), + maxSizeFromSetting + ) + .put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_STORAGE_PATH_KEY) + .getKey(), + env.nodePaths()[0].indicesPath.toString() + "/request_cache/" + 0 + ) + .build(); + + CacheConfig.Builder cacheConfigBuilder = new CacheConfig.Builder().setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setValueSerializer(new StringSerializer()) + .setWeigher(getWeigher()) + .setRemovalListener(listener) + .setSettings(settings) + .setDimensionNames(List.of(dimensionName)) + .setStatsTrackingEnabled(true); + if (putSizeInConfig) { + cacheConfigBuilder.setMaxSizeInBytes(maxSizeFromConfig); + } + + ICache.Factory cacheFactory = new EhcacheDiskCache.EhcacheDiskCacheFactory(); + return (EhcacheDiskCache) cacheFactory.create( + cacheConfigBuilder.build(), + CacheType.INDICES_REQUEST_CACHE, + null + ); + } + } + static class MockEhcahceDiskCache extends EhcacheDiskCache { public MockEhcahceDiskCache(Builder builder) { diff --git a/server/src/main/java/org/opensearch/common/cache/service/CacheService.java b/server/src/main/java/org/opensearch/common/cache/service/CacheService.java index 01da78ecec52e..9821afd6f0407 100644 --- a/server/src/main/java/org/opensearch/common/cache/service/CacheService.java +++ b/server/src/main/java/org/opensearch/common/cache/service/CacheService.java @@ -50,7 +50,7 @@ public ICache createCache(CacheConfig config, CacheType cache cacheType.getSettingPrefix() ); String storeName = cacheSettingForCacheType.get(settings); - if (!FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings) || (storeName == null || storeName.isBlank())) { + if (!pluggableCachingEnabled(cacheType, settings)) { // Condition 1: In case feature flag is off, we default to onHeap. // Condition 2: In case storeName is not explicitly mentioned, we assume user is looking to use older // settings, so we again fallback to onHeap to maintain backward compatibility. @@ -74,4 +74,15 @@ public NodeCacheStats stats(CommonStatsFlags flags) { } return new NodeCacheStats(statsMap, flags); } + + /** + * Check if pluggable caching is on, and if a store type is present for this cache type. + */ + public static boolean pluggableCachingEnabled(CacheType cacheType, Settings settings) { + Setting cacheSettingForCacheType = CacheSettings.CACHE_TYPE_STORE_NAME.getConcreteSettingForNamespace( + cacheType.getSettingPrefix() + ); + String storeName = cacheSettingForCacheType.get(settings); + return FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings) && storeName != null && !storeName.isBlank(); + } } 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 571383a9fce6a..3ca405b2e9c2e 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 @@ -17,6 +17,7 @@ import org.opensearch.common.cache.RemovalListener; import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; +import org.opensearch.common.cache.service.CacheService; import org.opensearch.common.cache.settings.CacheSettings; import org.opensearch.common.cache.stats.CacheStatsHolder; import org.opensearch.common.cache.stats.DefaultCacheStatsHolder; @@ -80,8 +81,8 @@ public OpenSearchOnHeapCache(Builder builder) { this.weigher = builder.getWeigher(); } - // package private for testing - long getMaximumWeight() { + // public for testing + public long getMaximumWeight() { return this.maximumWeight; } @@ -192,8 +193,12 @@ public ICache create(CacheConfig config, CacheType cacheType, ); long maxSizeInBytes = ((ByteSizeValue) settingList.get(MAXIMUM_SIZE_IN_BYTES_KEY).get(settings)).getBytes(); - if (config.getMaxSizeInBytes() > 0) { // If this is passed from upstream(like tieredCache), then use this - // instead. + if (config.getMaxSizeInBytes() > 0) { + /* + Use the cache config value if present. + This can be passed down from the TieredSpilloverCache when creating individual segments, + but is not passed in from the IRC if pluggable caching is on. + */ builder.setMaximumWeightInBytes(config.getMaxSizeInBytes()); } else { builder.setMaximumWeightInBytes(maxSizeInBytes); @@ -204,8 +209,7 @@ public ICache create(CacheConfig config, CacheType cacheType, builder.setNumberOfSegments(-1); // By default it will use 256 segments. } - String storeName = cacheSettingForCacheType.get(settings); - if (!FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings) || (storeName == null || storeName.isBlank())) { + if (!CacheService.pluggableCachingEnabled(cacheType, settings)) { // For backward compatibility as the user intent is to use older settings. builder.setMaximumWeightInBytes(config.getMaxSizeInBytes()); builder.setExpireAfterAccess(config.getExpireAfterAccess()); diff --git a/server/src/main/java/org/opensearch/common/cache/store/settings/OpenSearchOnHeapCacheSettings.java b/server/src/main/java/org/opensearch/common/cache/store/settings/OpenSearchOnHeapCacheSettings.java index 5a2964ad011bf..8ba356f9e0597 100644 --- a/server/src/main/java/org/opensearch/common/cache/store/settings/OpenSearchOnHeapCacheSettings.java +++ b/server/src/main/java/org/opensearch/common/cache/store/settings/OpenSearchOnHeapCacheSettings.java @@ -26,6 +26,7 @@ public class OpenSearchOnHeapCacheSettings { /** * Setting to define maximum size for the cache as a percentage of heap memory available. + * If this cache is used as a tier in a TieredSpilloverCache, this setting is ignored. * * Setting pattern: {cache_type}.opensearch_onheap.size */ diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 4dde4445cd483..a5226012baa77 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -124,6 +124,11 @@ public final class IndicesRequestCache implements RemovalListener INDICES_CACHE_QUERY_SIZE = Setting.memorySizeSetting( "indices.requests.cache.size", "1%", @@ -162,7 +167,6 @@ public final class IndicesRequestCache implements RemovalListener registeredClosedListeners = ConcurrentCollections.newConcurrentMap(); - private final ByteSizeValue size; private final TimeValue expire; private final ICache cache; private final ClusterService clusterService; @@ -183,10 +187,7 @@ public final class IndicesRequestCache implements RemovalListener, BytesReference> weigher = (k, v) -> k.ramBytesUsed(k.key.ramBytesUsed()) + v.ramBytesUsed(); this.cacheCleanupManager = new IndicesRequestCacheCleanupManager( threadPool, INDICES_REQUEST_CACHE_CLEANUP_INTERVAL_SETTING.get(settings), @@ -196,30 +197,43 @@ public final class IndicesRequestCache implements RemovalListener().setSettings(settings) - .setWeigher(weigher) - .setValueType(BytesReference.class) - .setKeyType(Key.class) - .setRemovalListener(this) - .setMaxSizeInBytes(sizeInBytes) // for backward compatibility - .setExpireAfterAccess(expire) // for backward compatibility - .setDimensionNames(List.of(INDEX_DIMENSION_NAME, SHARD_ID_DIMENSION_NAME)) - .setCachedResultParser((bytesReference) -> { - try { - return CachedQueryResult.getPolicyValues(bytesReference); - } catch (IOException e) { - // Set took time to -1, which will always be rejected by the policy. - return new CachedQueryResult.PolicyValues(-1); - } - }) - .setKeySerializer(new IRCKeyWriteableSerializer()) - .setValueSerializer(new BytesReferenceSerializer()) - .setClusterSettings(clusterService.getClusterSettings()) - .setStoragePath(nodeEnvironment.nodePaths()[0].path.toString() + "/request_cache") - .build(), - CacheType.INDICES_REQUEST_CACHE - ); + + CacheConfig config = getCacheConfig(settings, nodeEnvironment); + this.cache = cacheService.createCache(config, CacheType.INDICES_REQUEST_CACHE); + } + + // pkg-private for testing + CacheConfig getCacheConfig(Settings settings, NodeEnvironment nodeEnvironment) { + long sizeInBytes = INDICES_CACHE_QUERY_SIZE.get(settings).getBytes(); + ToLongBiFunction, BytesReference> weigher = (k, v) -> k.ramBytesUsed(k.key.ramBytesUsed()) + v.ramBytesUsed(); + CacheConfig.Builder configBuilder = new CacheConfig.Builder().setSettings(settings) + .setWeigher(weigher) + .setValueType(BytesReference.class) + .setKeyType(Key.class) + .setRemovalListener(this) + .setExpireAfterAccess(expire) // for backward compatibility + .setDimensionNames(List.of(INDEX_DIMENSION_NAME, SHARD_ID_DIMENSION_NAME)) + .setCachedResultParser((bytesReference) -> { + try { + return CachedQueryResult.getPolicyValues(bytesReference); + } catch (IOException e) { + // Set took time to -1, which will always be rejected by the policy. + return new CachedQueryResult.PolicyValues(-1); + } + }) + .setKeySerializer(new IRCKeyWriteableSerializer()) + .setValueSerializer(new BytesReferenceSerializer()) + .setClusterSettings(clusterService.getClusterSettings()) + .setStoragePath(nodeEnvironment.nodePaths()[0].path.toString() + "/request_cache"); + + if (!CacheService.pluggableCachingEnabled(CacheType.INDICES_REQUEST_CACHE, settings)) { + // If pluggable caching is not enabled, use the max size based on the IRC setting into the config. + // If pluggable caching is enabled, cache implementations instead determine their own sizes based on their own implementation + // size + // settings. + configBuilder.setMaxSizeInBytes(sizeInBytes); + } + return configBuilder.build(); } // package private for testing 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 45a7b273eb41e..5a989ad8ab777 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 @@ -15,6 +15,7 @@ import org.opensearch.common.cache.LoadAwareCacheLoader; import org.opensearch.common.cache.RemovalListener; import org.opensearch.common.cache.RemovalNotification; +import org.opensearch.common.cache.settings.CacheSettings; import org.opensearch.common.cache.stats.ImmutableCacheStats; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.cache.store.config.CacheConfig; @@ -105,35 +106,69 @@ public void testStatsWithoutPluggableCaches() throws Exception { } } - public void testWithCacheConfigSettings() { - MockRemovalListener listener = new MockRemovalListener<>(); - int maxKeys = between(10, 50); - ICache.Factory onHeapCacheFactory = new OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory(); - Settings settings = Settings.builder() - .put( - OpenSearchOnHeapCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) - .get(MAXIMUM_SIZE_IN_BYTES_KEY) - .getKey(), - 1000 + "b" // Setting some random value which shouldn't be honored. - ) + public void testWithCacheConfigSizeSettings_WhenPluggableCachingOff() { + // The "pluggable caching off" case can happen when the PLUGGABLE_CACHE setting is false, or if the store name is blank. + // The cache should get its size from the config, not the setting, in either case. + Settings.Builder settingsBuilder = Settings.builder().put(FeatureFlags.PLUGGABLE_CACHE, false); + long maxSizeFromSetting = between(1000, 2000); + long maxSizeFromConfig = between(3000, 4000); + OpenSearchOnHeapCache onHeapCache = setupMaxSizeTest(settingsBuilder, maxSizeFromSetting, maxSizeFromConfig, true); + assertEquals(maxSizeFromConfig, onHeapCache.getMaximumWeight()); + + Settings.Builder storeNameBlankSettingsBuilder = Settings.builder().put(FeatureFlags.PLUGGABLE_CACHE, true); + onHeapCache = setupMaxSizeTest(storeNameBlankSettingsBuilder, maxSizeFromSetting, maxSizeFromConfig, true); + assertEquals(maxSizeFromConfig, onHeapCache.getMaximumWeight()); + } + + public void testWithCacheConfigSettings_WhenPluggableCachingOn() { + // When pluggable caching is on, the cache should get its size from the config if present, and otherwise should get it from the + // setting. + Settings.Builder settingsBuilder = Settings.builder() .put(FeatureFlags.PLUGGABLE_CACHE, true) - .build(); + .put( + CacheSettings.getConcreteStoreNameSettingForCacheType(CacheType.INDICES_REQUEST_CACHE).getKey(), + OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory.NAME + ); + long maxSizeFromSetting = between(1000, 2000); + long maxSizeFromConfig = between(3000, 4000); + OpenSearchOnHeapCache onHeapCache = setupMaxSizeTest(settingsBuilder, maxSizeFromSetting, maxSizeFromConfig, false); + assertEquals(maxSizeFromSetting, onHeapCache.getMaximumWeight()); + + onHeapCache = setupMaxSizeTest(settingsBuilder, maxSizeFromSetting, maxSizeFromConfig, true); + assertEquals(maxSizeFromConfig, onHeapCache.getMaximumWeight()); + } - CacheConfig cacheConfig = new CacheConfig.Builder().setKeyType(String.class) + private OpenSearchOnHeapCache setupMaxSizeTest( + Settings.Builder settingsBuilder, + long maxSizeFromSetting, + long maxSizeFromConfig, + boolean putSizeInConfig + ) { + MockRemovalListener listener = new MockRemovalListener<>(); + settingsBuilder.put( + OpenSearchOnHeapCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(MAXIMUM_SIZE_IN_BYTES_KEY) + .getKey(), + maxSizeFromSetting + "b" + ); + + CacheConfig.Builder cacheConfigBuilder = new CacheConfig.Builder().setKeyType(String.class) .setValueType(String.class) .setWeigher((k, v) -> keyValueSize) .setRemovalListener(listener) - .setSettings(settings) + .setSettings(settingsBuilder.build()) .setDimensionNames(dimensionNames) - .setMaxSizeInBytes(maxKeys * keyValueSize) // this should get honored - .setStatsTrackingEnabled(true) - .build(); - OpenSearchOnHeapCache onHeapCache = (OpenSearchOnHeapCache) onHeapCacheFactory.create( - cacheConfig, + .setStatsTrackingEnabled(true); + if (putSizeInConfig) { + cacheConfigBuilder.setMaxSizeInBytes(maxSizeFromConfig); + } + + ICache.Factory onHeapCacheFactory = new OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory(); + return (OpenSearchOnHeapCache) onHeapCacheFactory.create( + cacheConfigBuilder.build(), CacheType.INDICES_REQUEST_CACHE, null ); - assertEquals(maxKeys * keyValueSize, onHeapCache.getMaximumWeight()); } private void assertZeroStats(ImmutableCacheStatsHolder stats) { diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 1a3aece74b3e2..3a2b12bc239b2 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -53,12 +53,16 @@ import org.opensearch.cluster.routing.ShardRoutingHelper; import org.opensearch.cluster.routing.UnassignedInfo; import org.opensearch.common.CheckedSupplier; +import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.ICacheKey; import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.module.CacheModule; +import org.opensearch.common.cache.settings.CacheSettings; import org.opensearch.common.cache.stats.ImmutableCacheStats; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; +import org.opensearch.common.cache.store.OpenSearchOnHeapCache; +import org.opensearch.common.cache.store.config.CacheConfig; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.lucene.index.OpenSearchDirectoryReader; import org.opensearch.common.settings.Settings; @@ -852,6 +856,40 @@ public void testAddingToCleanupKeyToCountMapWorksAppropriatelyWithMultipleThread assertFalse(concurrentModificationExceptionDetected.get()); } + public void testCacheMaxSize_WhenPluggableCachingOff() throws Exception { + // If pluggable caching is off, the IRC should put a max size value into the cache config that it uses to create its cache. + threadPool = getThreadPool(); + long cacheSize = 1000; + Settings settings = Settings.builder().put(INDICES_CACHE_QUERY_SIZE.getKey(), cacheSize + "b").build(); + cache = getIndicesRequestCache(settings); + CacheConfig config; + try (NodeEnvironment env = newNodeEnvironment(settings)) { + // For the purposes of this test it doesn't matter if the node environment matches the one used in the constructor + config = cache.getCacheConfig(settings, env); + } + assertEquals(cacheSize, (long) config.getMaxSizeInBytes()); + } + + public void testCacheMaxSize_WhenPluggableCachingOn() throws Exception { + // If pluggable caching is on, and a store name is present, the IRC should NOT put a max size value into the cache config. + threadPool = getThreadPool(); + Settings settings = Settings.builder() + .put(INDICES_CACHE_QUERY_SIZE.getKey(), 1000 + "b") + .put(FeatureFlags.PLUGGABLE_CACHE, true) + .put( + CacheSettings.getConcreteStoreNameSettingForCacheType(CacheType.INDICES_REQUEST_CACHE).getKey(), + OpenSearchOnHeapCache.OpenSearchOnHeapCacheFactory.NAME + ) + .build(); + cache = getIndicesRequestCache(settings); + CacheConfig config; + try (NodeEnvironment env = newNodeEnvironment(settings)) { + // For the purposes of this test it doesn't matter if the node environment matches the one used in the constructor + config = cache.getCacheConfig(settings, env); + } + assertEquals(0, (long) config.getMaxSizeInBytes()); + } + private IndicesRequestCache getIndicesRequestCache(Settings settings) throws IOException { IndicesService indicesService = getInstanceFromNode(IndicesService.class); try (NodeEnvironment env = newNodeEnvironment(settings)) { From 3d1fe7350b0fb8bbb293e036d704ed95d4942dd6 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 13 Nov 2024 16:42:19 -0800 Subject: [PATCH 2/4] Changelog Signed-off-by: Peter Alfonsi --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e46628249c91e..4ce9d15f79556 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Support retrieving doc values of unsigned long field ([#16543](https://github.com/opensearch-project/OpenSearch/pull/16543)) - Fix rollover alias supports restored searchable snapshot index([#16483](https://github.com/opensearch-project/OpenSearch/pull/16483)) - Fix permissions error on scripted query against remote snapshot ([#16544](https://github.com/opensearch-project/OpenSearch/pull/16544)) +- Fix max request cache size settings not working properly with pluggable caching ([#16636](https://github.com/opensearch-project/OpenSearch/pull/16636)) ### Security From 76ad2ef113c5f1dcffb81817541b0e349378a638 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 14 Nov 2024 12:10:15 -0800 Subject: [PATCH 3/4] Deprecate original IRC size setting Signed-off-by: Peter Alfonsi --- .../cache/store/disk/EhCacheDiskCacheTests.java | 1 - .../org/opensearch/indices/IndicesRequestCache.java | 8 +++++--- .../common/settings/MemorySizeSettingsTests.java | 3 +++ .../opensearch/indices/IndicesRequestCacheTests.java | 10 ++++++++++ 4 files changed, 18 insertions(+), 4 deletions(-) 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 cfa0940101ad1..5ff78367c4d35 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 @@ -53,7 +53,6 @@ import java.util.function.ToLongBiFunction; import org.ehcache.PersistentCacheManager; -import org.ehcache.impl.serialization.StringSerializer; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_LISTENER_MODE_SYNC_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_MAX_SIZE_IN_BYTES_KEY; diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index a5226012baa77..3f6f04a8d37f6 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -128,11 +128,14 @@ public final class IndicesRequestCache implements RemovalListener INDICES_CACHE_QUERY_SIZE = Setting.memorySizeSetting( "indices.requests.cache.size", "1%", - Property.NodeScope + Property.NodeScope, + Property.Deprecated ); public static final Setting INDICES_CACHE_QUERY_EXPIRE = Setting.positiveTimeSetting( "indices.requests.cache.expire", @@ -229,8 +232,7 @@ CacheConfig getCacheConfig(Settings settings, NodeEnvironme if (!CacheService.pluggableCachingEnabled(CacheType.INDICES_REQUEST_CACHE, settings)) { // If pluggable caching is not enabled, use the max size based on the IRC setting into the config. // If pluggable caching is enabled, cache implementations instead determine their own sizes based on their own implementation - // size - // settings. + // size settings. configBuilder.setMaxSizeInBytes(sizeInBytes); } return configBuilder.build(); diff --git a/server/src/test/java/org/opensearch/common/settings/MemorySizeSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/MemorySizeSettingsTests.java index 78782112be844..c90924cfc0fd1 100644 --- a/server/src/test/java/org/opensearch/common/settings/MemorySizeSettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/MemorySizeSettingsTests.java @@ -81,6 +81,9 @@ public void testIndicesRequestCacheSetting() { "indices.requests.cache.size", new ByteSizeValue((long) (JvmInfo.jvmInfo().getMem().getHeapMax().getBytes() * 0.01)) ); + assertWarnings( + "[indices.requests.cache.size] setting was deprecated in OpenSearch and will be removed in a future release! See the breaking changes documentation for the next major version." + ); } public void testCircuitBreakerSettings() { diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 3a2b12bc239b2..e83ca247b6a1d 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -868,6 +868,7 @@ public void testCacheMaxSize_WhenPluggableCachingOff() throws Exception { config = cache.getCacheConfig(settings, env); } assertEquals(cacheSize, (long) config.getMaxSizeInBytes()); + allowDeprecationWarning(); } public void testCacheMaxSize_WhenPluggableCachingOn() throws Exception { @@ -888,6 +889,7 @@ public void testCacheMaxSize_WhenPluggableCachingOn() throws Exception { config = cache.getCacheConfig(settings, env); } assertEquals(0, (long) config.getMaxSizeInBytes()); + allowDeprecationWarning(); } private IndicesRequestCache getIndicesRequestCache(Settings settings) throws IOException { @@ -1133,6 +1135,7 @@ public void testEviction() throws Exception { assertEquals(2, cache.count()); assertEquals(1, indexShard.requestCache().stats().getEvictions()); IOUtils.close(reader, secondReader, thirdReader, environment); + allowDeprecationWarning(); } public void testClearAllEntityIdentity() throws Exception { @@ -1410,6 +1413,7 @@ public void testGetOrComputeConcurrentlyWithMultipleIndices() throws Exception { } IOUtils.close(cache); executorService.shutdownNow(); + allowDeprecationWarning(); } public void testDeleteAndCreateIndexShardOnSameNodeAndVerifyStats() throws Exception { @@ -1578,6 +1582,12 @@ public static String generateString(int length) { return sb.toString(); } + private void allowDeprecationWarning() { + assertWarnings( + "[indices.requests.cache.size] setting was deprecated in OpenSearch and will be removed in a future release! See the breaking changes documentation for the next major version." + ); + } + private class TestBytesReference extends AbstractBytesReference { int dummyValue; From 8ea8f077cc977ab38d6eed081b7dc87843dd1e6e Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 17 Dec 2024 13:45:21 -0800 Subject: [PATCH 4/4] spotlessApply Signed-off-by: Peter Alfonsi --- .../opensearch/cache/common/tier/TieredSpilloverCacheTests.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java index 4fa4ee64c1f0e..996c3bfd71100 100644 --- a/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java +++ b/modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java @@ -2112,6 +2112,7 @@ public void testTieredCacheDefaultSegmentCount() { assertEquals(TieredSpilloverCacheSettings.defaultSegments(), tieredSpilloverCache.getNumberOfSegments()); assertTrue(VALID_SEGMENT_COUNT_VALUES.contains(tieredSpilloverCache.getNumberOfSegments())); } + public void testDropStatsForDimensions() throws Exception { int onHeapCacheSize = randomIntBetween(300, 600); int diskCacheSize = randomIntBetween(700, 1200); @@ -2165,6 +2166,7 @@ public void testDropStatsForDimensions() throws Exception { tieredSpilloverCache.invalidate(dropDimensionsKey); assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), tieredSpilloverCache.stats().getTotalStats()); } + public void testSegmentSizesWhenUsingFactory() { // The TSC's tier size settings, TIERED_SPILLOVER_ONHEAP_STORE_SIZE and TIERED_SPILLOVER_DISK_STORE_SIZE, // should always be respected, overriding the individual implementation's size settings if present