Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix] [Tiered Caching] Make ehcache disk cache size setting be ByteSizeValue rather than long #13902

Closed
wants to merge 5 commits into from
Closed

[Bugfix] [Tiered Caching] Make ehcache disk cache size setting be ByteSizeValue rather than long #13902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7ce5180
Make ehcache size setting take ByteSizeValue
May 30, 2024
294cdac
Merge remote-tracking branch 'upstream/main' into ehcache-size-settin…
Jul 17, 2024
4510966
rerun failed security check
Jul 17, 2024
934eefd
Changed logic to use new setting and deprecate old one
Jul 26, 2024
27c7b22
Merge remote-tracking branch 'upstream/main' into ehcache-size-settin…
Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions ...s/cache-ehcache/src/internalClusterTest/java/org/opensearch/cache/EhcacheDiskCacheIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
22 changes: 19 additions & 3 deletions plugins/cache-ehcache/src/main/java/org/opensearch/cache/EhcacheDiskCacheSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Long> 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<ByteSizeValue> 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause any issue with upgrade path where older nodes have the setting as long type vs new nodes has it as ByteSizeValue type ? Probably not given it is NodeScope but wanted to double confirm on this.

Copy link
Collaborator

@jainankitk jainankitk Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause any issue with upgrade path where older nodes have the setting as long type vs new nodes has it as ByteSizeValue type ? Probably not given it is NodeScope but wanted to double confirm on this.

@sohami - Thinking again on this, it will break for old nodes. Since once the setting is preserved as memorySizeSetting in the cluster state, old nodes will not be able to apply the latest cluster state and cluster manager will kick those nodes out of the cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a new change so that there's a new ByteSizeSetting "max_size" rather than "max_size_in_bytes", which has been deprecated. If the new setting is present it uses that value. If not, it looks for the deprecated setting and uses that. If neither are present it uses the default value.

I'm not totally sure this complexity is worth it, it might just be better to keep the long setting as it was before. Let me know what you think @jainankitk @sohami

);

/**
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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
);
Expand Down
25 changes: 24 additions & 1 deletion plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*/
Expand All @@ -689,6 +696,22 @@ public static class EhcacheDiskCacheFactory implements ICache.Factory {
*/
public EhcacheDiskCacheFactory() {}

private long getMaxSizeInBytes(Map<String, Setting<?>> 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 <K, V> ICache<K, V> create(CacheConfig<K, V> config, CacheType cacheType, Map<String, Factory> cacheFactories) {
Expand Down Expand Up @@ -721,7 +744,7 @@ public <K, V> ICache<K, V> create(CacheConfig<K, V> 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();
}
Expand Down
138 changes: 136 additions & 2 deletions ...ns/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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<String, String> removalListener = new MockRemovalListener<>();
try (NodeEnvironment env = newNodeEnvironment(Settings.EMPTY)) {
// First try various valid options for the cache size setting
List<Settings> validSettings = new ArrayList<>();
List<Long> 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<String> 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<String, String> ehcacheTest = ehcacheFactory.create(
new CacheConfig.Builder<String, String>().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<String, String>) ehcacheTest).getMaxWeightInBytes());
ehcacheTest.close();
}

// Next try an invalid one and show we can't construct the disk cache
assertThrows(IllegalArgumentException.class, () -> {
ICache<String, String> ehcacheTest = ehcacheFactory.create(
new CacheConfig.Builder<String, String>().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<String, String> removalListener = new MockRemovalListener<>();
Expand Down
Loading