From 934eefd48102c6cdbc4a089e8dc823f9774ab341 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 26 Jul 2024 09:40:21 -0700 Subject: [PATCH] Changed logic to use new setting and deprecate old one Signed-off-by: Peter Alfonsi --- .../opensearch/cache/EhcacheDiskCacheIT.java | 6 +-- .../cache/EhcacheDiskCacheSettings.java | 21 ++++++-- .../cache/store/disk/EhcacheDiskCache.java | 19 ++++++- .../store/disk/EhCacheDiskCacheTests.java | 52 +++++++++++++++---- 4 files changed, 81 insertions(+), 17 deletions(-) diff --git a/plugins/cache-ehcache/src/internalClusterTest/java/org/opensearch/cache/EhcacheDiskCacheIT.java b/plugins/cache-ehcache/src/internalClusterTest/java/org/opensearch/cache/EhcacheDiskCacheIT.java index 18f5ee9ef83ae..6d2e4f568c253 100644 --- a/plugins/cache-ehcache/src/internalClusterTest/java/org/opensearch/cache/EhcacheDiskCacheIT.java +++ b/plugins/cache-ehcache/src/internalClusterTest/java/org/opensearch/cache/EhcacheDiskCacheIT.java @@ -54,7 +54,7 @@ import static org.opensearch.cache.EhcacheDiskCacheSettings.DEFAULT_CACHE_SIZE_IN_BYTES; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_CACHE_EXPIRE_AFTER_ACCESS_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_LISTENER_MODE_SYNC_KEY; -import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_MAX_SIZE_IN_BYTES_KEY; +import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_MAX_SIZE_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_STORAGE_PATH_KEY; import static org.opensearch.indices.IndicesService.INDICES_CACHE_CLEAN_INTERVAL_SETTING; import static org.opensearch.search.aggregations.AggregationBuilders.dateHistogram; @@ -100,9 +100,7 @@ private Settings defaultSettings(long sizeInBytes, TimeValue expirationTime) { true ) .put( - EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) - .get(DISK_MAX_SIZE_IN_BYTES_KEY) - .getKey(), + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE).get(DISK_MAX_SIZE_KEY).getKey(), new ByteSizeValue(sizeInBytes) ) .put( 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 7850bade98d73..8854e9f80faa3 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,10 +101,18 @@ public class EhcacheDiskCacheSettings { ); /** - * Disk cache max size setting. + * Deprecated: Disk cache max size setting, which takes a long. */ - public static final Setting.AffixSetting DISK_CACHE_MAX_SIZE_IN_BYTES_SETTING = Setting.suffixKeySetting( + public static final Setting.AffixSetting DISK_CACHE_MAX_SIZE_IN_BYTES_SETTING = Setting.suffixKeySetting( EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + ".max_size_in_bytes", + (key) -> Setting.longSetting(key, DEFAULT_CACHE_SIZE_IN_BYTES, NodeScope, Setting.Property.Deprecated) + ); + + /** + * Disk cache max size setting, which takes a ByteSizeValue. + */ + public static final Setting.AffixSetting DISK_CACHE_MAX_SIZE_SETTING = Setting.suffixKeySetting( + EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + ".max_size", (key) -> Setting.memorySizeSetting(key, new ByteSizeValue(DEFAULT_CACHE_SIZE_IN_BYTES), NodeScope) ); @@ -121,9 +129,14 @@ public class EhcacheDiskCacheSettings { */ public static final String DISK_SEGMENT_KEY = "disk_segment"; /** - * Key for max size. + * Deprecated: Key for max size. */ public static final String DISK_MAX_SIZE_IN_BYTES_KEY = "max_size_in_bytes"; + + /** + * Key for max size. + */ + public static final String DISK_MAX_SIZE_KEY = "max_size"; /** * Key for expire after access. */ @@ -177,6 +190,8 @@ public class EhcacheDiskCacheSettings { DISK_STORAGE_PATH_SETTING, DISK_MAX_SIZE_IN_BYTES_KEY, DISK_CACHE_MAX_SIZE_IN_BYTES_SETTING, + DISK_MAX_SIZE_KEY, + DISK_CACHE_MAX_SIZE_SETTING, DISK_LISTENER_MODE_SYNC_KEY, DISK_CACHE_LISTENER_MODE_SYNC_SETTING ); 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 523bde96d4c38..34dcab9c123f6 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 @@ -83,6 +83,7 @@ import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_CACHE_EXPIRE_AFTER_ACCESS_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_LISTENER_MODE_SYNC_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_MAX_SIZE_IN_BYTES_KEY; +import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_MAX_SIZE_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_SEGMENT_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_STORAGE_PATH_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_WRITE_CONCURRENCY_KEY; @@ -675,6 +676,22 @@ public static class EhcacheDiskCacheFactory implements ICache.Factory { */ public EhcacheDiskCacheFactory() {} + private long getMaxSizeInBytes(Map> settingList, Settings settings) { + /* + Use DISK_CACHE_MAX_SIZE_SETTING. If absent, check for the deprecated DISK_CACHE_MAX_SIZE_IN_BYTES_SETTING + to use as a fallback. If that's also absent, use the default value of DISK_CACHE_MAX_SIZE_SETTING. + */ + if (settingList.get(DISK_MAX_SIZE_KEY).exists(settings)) { + return ((ByteSizeValue) settingList.get(DISK_MAX_SIZE_KEY).get(settings)).getBytes(); + } else { + if (settingList.get(DISK_MAX_SIZE_IN_BYTES_KEY).exists(settings)) { + return (long) settingList.get(DISK_MAX_SIZE_IN_BYTES_KEY).get(settings); + } else { + return ((ByteSizeValue) settingList.get(DISK_MAX_SIZE_KEY).get(settings)).getBytes(); + } + } + } + @Override @SuppressWarnings({ "unchecked" }) // Required to ensure the serializers output byte[] public ICache create(CacheConfig config, CacheType cacheType, Map cacheFactories) { @@ -707,7 +724,7 @@ public ICache create(CacheConfig config, CacheType cacheType, .setWeigher(config.getWeigher()) .setRemovalListener(config.getRemovalListener()) .setExpireAfterAccess((TimeValue) settingList.get(DISK_CACHE_EXPIRE_AFTER_ACCESS_KEY).get(settings)) - .setMaximumWeightInBytes(((ByteSizeValue) settingList.get(DISK_MAX_SIZE_IN_BYTES_KEY).get(settings)).getBytes()) + .setMaximumWeightInBytes(getMaxSizeInBytes(settingList, settings)) .setSettings(settings) .build(); } 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 00135379a99f4..c1ed2038c2f44 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 @@ -49,8 +49,12 @@ import java.util.concurrent.Phaser; import java.util.function.ToLongBiFunction; +import org.ehcache.impl.serialization.StringSerializer; + +import static org.opensearch.cache.EhcacheDiskCacheSettings.DEFAULT_CACHE_SIZE_IN_BYTES; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_LISTENER_MODE_SYNC_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_MAX_SIZE_IN_BYTES_KEY; +import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_MAX_SIZE_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_STORAGE_PATH_KEY; import static org.hamcrest.CoreMatchers.instanceOf; @@ -128,7 +132,7 @@ public void testBasicGetAndPutUsingFactory() throws IOException { Settings.builder() .put( EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) - .get(DISK_MAX_SIZE_IN_BYTES_KEY) + .get(DISK_MAX_SIZE_KEY) .getKey(), new ByteSizeValue(CACHE_SIZE_IN_BYTES) ) @@ -895,15 +899,15 @@ public void testCacheSizeSetting() throws Exception { new ByteSizeValue(10, ByteSizeUnit.GB).getBytes(), new ByteSizeValue(1, ByteSizeUnit.GB).getBytes(), new ByteSizeValue(50000000, ByteSizeUnit.BYTES).getBytes(), - EhcacheDiskCacheSettings.DEFAULT_CACHE_SIZE_IN_BYTES + DEFAULT_CACHE_SIZE_IN_BYTES, + 1_000_000L, + 2_000_000L ); // The expected size of the cache produced by each of the settings in validSettings // Should be able to pass a ByteSizeValue directly validSettings.add( getSettingsExceptSize(env).put( - EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) - .get(DISK_MAX_SIZE_IN_BYTES_KEY) - .getKey(), + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE).get(DISK_MAX_SIZE_KEY).getKey(), new ByteSizeValue(CACHE_SIZE_IN_BYTES) ).build() ); @@ -914,17 +918,44 @@ public void testCacheSizeSetting() throws Exception { validSettings.add( getSettingsExceptSize(env).put( EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) - .get(DISK_MAX_SIZE_IN_BYTES_KEY) + .get(DISK_MAX_SIZE_KEY) .getKey(), validString ).build() ); } - // Passing in settings missing a size value should give us the default + // Passing in settings missing either size setting should give us the default validSettings.add(getSettingsExceptSize(env).build()); - assertEquals(validSettings.size(), expectedCacheSizes.size()); + // Deprecated setting present, correct setting absent + validSettings.add( + getSettingsExceptSize(env).put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_MAX_SIZE_IN_BYTES_KEY) + .getKey(), + 1_000_000L + ).build() + ); + + // Both settings present, the correct one should be used + validSettings.add( + getSettingsExceptSize(env).put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_MAX_SIZE_IN_BYTES_KEY) + .getKey(), + 1_000_000L + ) + .put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_MAX_SIZE_KEY) + .getKey(), + new ByteSizeValue(2_000_000L) + ) + .build() + ); + + assertEquals(validSettings.size(), expectedCacheSizes.size()); ICache.Factory ehcacheFactory = new EhcacheDiskCache.EhcacheDiskCacheFactory(); for (int i = 0; i < validSettings.size(); i++) { @@ -958,7 +989,7 @@ public void testCacheSizeSetting() throws Exception { .setSettings( getSettingsExceptSize(env).put( EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) - .get(DISK_MAX_SIZE_IN_BYTES_KEY) + .get(DISK_MAX_SIZE_KEY) .getKey(), "1000000" ).build() @@ -968,6 +999,9 @@ public void testCacheSizeSetting() throws Exception { Map.of() ); }); + assertWarnings( + "[indices.requests.cache.ehcache_disk.max_size_in_bytes] setting was deprecated in OpenSearch and will be removed in a future release! See the breaking changes documentation for the next major version." + ); } }