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 909a493c0734f..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 @@ -28,6 +28,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.FeatureFlags; +import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.env.NodeEnvironment; import org.opensearch.index.cache.request.RequestCacheStats; import org.opensearch.index.query.QueryBuilders; @@ -53,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; @@ -99,10 +100,8 @@ private Settings defaultSettings(long sizeInBytes, TimeValue expirationTime) { true ) .put( - EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) - .get(DISK_MAX_SIZE_IN_BYTES_KEY) - .getKey(), - sizeInBytes + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE).get(DISK_MAX_SIZE_KEY).getKey(), + new ByteSizeValue(sizeInBytes) ) .put( EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) 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 d173155080f6a..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 @@ -12,6 +12,7 @@ import org.opensearch.common.cache.CacheType; import org.opensearch.common.settings.Setting; import org.opensearch.common.unit.TimeValue; +import org.opensearch.core.common.unit.ByteSizeValue; import java.util.HashMap; import java.util.Map; @@ -100,11 +101,19 @@ 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( EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + ".max_size_in_bytes", - (key) -> Setting.longSetting(key, DEFAULT_CACHE_SIZE_IN_BYTES, NodeScope) + (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) ); /** @@ -120,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. */ @@ -176,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 4a95b04de3952..8380eaf2e9199 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 @@ -35,6 +35,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.io.IOUtils; +import org.opensearch.core.common.unit.ByteSizeValue; import java.io.File; import java.io.IOException; @@ -81,6 +82,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; @@ -674,6 +676,11 @@ private V deserializeValue(ByteArrayWrapper binary) { return valueSerializer.deserialize(binary.value); } + // For testing + long getMaxWeightInBytes() { + return maxWeightInBytes; + } + /** * Factory to create an ehcache disk cache. */ @@ -689,6 +696,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) { @@ -721,7 +744,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((Long) settingList.get(DISK_MAX_SIZE_IN_BYTES_KEY).get(settings)) + .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 2bc24227bb513..b1aa8d09907c2 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 @@ -29,6 +29,8 @@ import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.common.bytes.CompositeBytesReference; +import org.opensearch.core.common.unit.ByteSizeUnit; +import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.env.NodeEnvironment; import org.opensearch.test.OpenSearchSingleNodeTestCase; @@ -50,10 +52,12 @@ import java.util.concurrent.Phaser; import java.util.function.ToLongBiFunction; +import static org.opensearch.cache.EhcacheDiskCacheSettings.DEFAULT_CACHE_SIZE_IN_BYTES; import org.ehcache.PersistentCacheManager; 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.cache.store.disk.EhcacheDiskCache.MINIMUM_MAX_SIZE_IN_BYTES; import static org.hamcrest.CoreMatchers.instanceOf; @@ -136,9 +140,9 @@ 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(), - CACHE_SIZE_IN_BYTES + new ByteSizeValue(CACHE_SIZE_IN_BYTES) ) .put( EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) @@ -892,6 +896,136 @@ public void testStatsTrackingDisabled() throws Exception { } } + public void testCacheSizeSetting() throws Exception { + // Cache size setting should be a ByteSizeValue or parseable as bytes, not a long + MockRemovalListener removalListener = new MockRemovalListener<>(); + try (NodeEnvironment env = newNodeEnvironment(Settings.EMPTY)) { + // First try various valid options for the cache size setting + List validSettings = new ArrayList<>(); + List expectedCacheSizes = List.of( + (long) CACHE_SIZE_IN_BYTES, + new ByteSizeValue(10, ByteSizeUnit.GB).getBytes(), + new ByteSizeValue(1, ByteSizeUnit.GB).getBytes(), + new ByteSizeValue(50000000, ByteSizeUnit.BYTES).getBytes(), + 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_KEY).getKey(), + new ByteSizeValue(CACHE_SIZE_IN_BYTES) + ).build() + ); + + // Should also be able to pass strings which can be parsed as bytes + List validSettingStrings = List.of("10GB", "1G", "50000000B"); + for (String validString : validSettingStrings) { + validSettings.add( + getSettingsExceptSize(env).put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_MAX_SIZE_KEY) + .getKey(), + validString + ).build() + ); + } + + // Passing in settings missing either size setting should give us the default + validSettings.add(getSettingsExceptSize(env).build()); + + // 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++) { + ICache ehcacheTest = ehcacheFactory.create( + new CacheConfig.Builder().setValueType(String.class) + .setKeyType(String.class) + .setRemovalListener(removalListener) + .setKeySerializer(new StringSerializer()) + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setWeigher(getWeigher()) + .setSettings(validSettings.get(i)) + .build(), + CacheType.INDICES_REQUEST_CACHE, + Map.of() + ); + assertEquals((long) expectedCacheSizes.get(i), ((EhcacheDiskCache) ehcacheTest).getMaxWeightInBytes()); + ehcacheTest.close(); + } + + // Next try an invalid one and show we can't construct the disk cache + assertThrows(IllegalArgumentException.class, () -> { + ICache ehcacheTest = ehcacheFactory.create( + new CacheConfig.Builder().setValueType(String.class) + .setKeyType(String.class) + .setRemovalListener(removalListener) + .setKeySerializer(new StringSerializer()) + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setWeigher(getWeigher()) + .setSettings( + getSettingsExceptSize(env).put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_MAX_SIZE_KEY) + .getKey(), + "1000000" + ).build() + ) + .build(), + CacheType.INDICES_REQUEST_CACHE, + 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." + ); + } + } + + private Settings.Builder getSettingsExceptSize(NodeEnvironment env) { + return Settings.builder() + .put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE).get(DISK_STORAGE_PATH_KEY).getKey(), + env.nodePaths()[0].indicesPath.toString() + "/request_cache" + ) + .put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_LISTENER_MODE_SYNC_KEY) + .getKey(), + true + ); + } public void testDiskCacheFilesAreClearedUpDuringCloseAndInitialization() throws Exception { Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>();