From 1f7f460c4ddef381e653f2c77612a2a7816a05af Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 8 Mar 2024 14:12:38 -0800 Subject: [PATCH] Addressed Sagar's comment Signed-off-by: Peter Alfonsi --- .../cache/EhcacheDiskCacheSettings.java | 19 ++-- .../cache/keystore/DummyKeystore.java | 57 +++++++++++ .../cache/keystore/RBMIntKeyLookupStore.java | 3 + .../cache/store/disk/EhcacheDiskCache.java | 29 +++--- .../store/disk/EhCacheDiskCacheTests.java | 94 +++++++++++++++++++ 5 files changed, 182 insertions(+), 20 deletions(-) create mode 100644 plugins/cache-ehcache/src/main/java/org/opensearch/cache/keystore/DummyKeystore.java 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 bdcc03d2e4ab8..5eef831047e43 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 @@ -8,6 +8,7 @@ package org.opensearch.cache; +import org.opensearch.cache.keystore.RBMIntKeyLookupStore; import org.opensearch.cache.store.disk.EhcacheDiskCache; import org.opensearch.common.cache.CacheType; import org.opensearch.common.settings.Setting; @@ -114,16 +115,16 @@ public class EhcacheDiskCacheSettings { /** * Defines whether to use an in-memory keystore to check for probable presence of keys before having to go to disk. */ - public static final Setting.AffixSetting USE_RBM_KEYSTORE_SETTING = Setting.suffixKeySetting( - EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + "use_keystore", - (key) -> Setting.boolSetting(key, true, NodeScope) + public static final Setting.AffixSetting USE_KEYSTORE_SETTING = Setting.suffixKeySetting( + EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + ".keystore", + (key) -> Setting.simpleString(key, RBMIntKeyLookupStore.KEYSTORE_NAME, NodeScope) ); /** * Defines the max size of the RBM keystore if used (as a percentage of heap memory) */ - public static final Setting.AffixSetting RBM_KEYSTORE_SIZE_SETTING = Setting.suffixKeySetting( - EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + "keystore_size", + public static final Setting.AffixSetting KEYSTORE_SIZE_SETTING = Setting.suffixKeySetting( + EhcacheDiskCache.EhcacheDiskCacheFactory.EHCACHE_DISK_CACHE_NAME + ".keystore_size", (key) -> Setting.memorySizeSetting(key, "0.05%", NodeScope) ); @@ -170,11 +171,11 @@ public class EhcacheDiskCacheSettings { /** * Key for whether to use RBM keystore */ - public static final String USE_RBM_KEYSTORE_KEY = "use_keystore"; + public static final String USE_KEYSTORE_KEY = "use_keystore"; /** * Key for the keystore size in bytes */ - public static final String RBM_KEYSTORE_SIZE_KEY = "keystore_size"; + public static final String KEYSTORE_SIZE_KEY = "keystore_size"; /** * Map of key to setting. @@ -189,8 +190,8 @@ public class EhcacheDiskCacheSettings { Map.entry(DISK_STORAGE_PATH_KEY, DISK_STORAGE_PATH_SETTING), Map.entry(DISK_MAX_SIZE_IN_BYTES_KEY, DISK_CACHE_MAX_SIZE_IN_BYTES_SETTING), Map.entry(DISK_LISTENER_MODE_SYNC_KEY, DISK_CACHE_LISTENER_MODE_SYNC_SETTING), - Map.entry(USE_RBM_KEYSTORE_KEY, USE_RBM_KEYSTORE_SETTING), - Map.entry(RBM_KEYSTORE_SIZE_KEY, RBM_KEYSTORE_SIZE_SETTING) + Map.entry(USE_KEYSTORE_KEY, USE_KEYSTORE_SETTING), + Map.entry(KEYSTORE_SIZE_KEY, KEYSTORE_SIZE_SETTING) ); /** diff --git a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/keystore/DummyKeystore.java b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/keystore/DummyKeystore.java new file mode 100644 index 0000000000000..cb679a009312b --- /dev/null +++ b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/keystore/DummyKeystore.java @@ -0,0 +1,57 @@ +/* + * 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.cache.keystore; + +import org.opensearch.common.cache.keystore.KeyLookupStore; + +/** + * A dummy keystore, which will always report that a key is contained in it. + */ +public class DummyKeystore implements KeyLookupStore { + @Override + public boolean add(Integer value) { + return true; + } + + @Override + public boolean contains(Integer value) { + return true; + } + + @Override + public boolean remove(Integer value) { + return true; + } + + @Override + public int getSize() { + return 0; + } + + @Override + public long getMemorySizeInBytes() { + return 0; + } + + @Override + public long getMemorySizeCapInBytes() { + return 0; + } + + @Override + public boolean isFull() { + return false; + } + + @Override + public void regenerateStore(Integer[] newValues) {} + + @Override + public void clear() {} +} diff --git a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/keystore/RBMIntKeyLookupStore.java b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/keystore/RBMIntKeyLookupStore.java index bf3b7e83fdc17..617390fb3921f 100644 --- a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/keystore/RBMIntKeyLookupStore.java +++ b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/keystore/RBMIntKeyLookupStore.java @@ -28,6 +28,9 @@ * The store estimates its memory footprint and will stop adding more values once it reaches its memory cap. */ public class RBMIntKeyLookupStore implements KeyLookupStore { + /** Used in settings to distinguish between keystore types. */ + public static final String KEYSTORE_NAME = "rbm"; + /** * An enum representing modulo values for use in the keystore */ 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 a71ebb397a1d2..da089a5ddd87b 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 @@ -12,6 +12,7 @@ import org.apache.logging.log4j.Logger; import org.opensearch.OpenSearchException; import org.opensearch.cache.EhcacheDiskCacheSettings; +import org.opensearch.cache.keystore.DummyKeystore; import org.opensearch.cache.keystore.RBMIntKeyLookupStore; import org.opensearch.common.SuppressForbidden; import org.opensearch.common.annotation.ExperimentalApi; @@ -70,8 +71,8 @@ import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_WRITE_CONCURRENCY_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_WRITE_MAXIMUM_THREADS_KEY; import static org.opensearch.cache.EhcacheDiskCacheSettings.DISK_WRITE_MIN_THREADS_KEY; -import static org.opensearch.cache.EhcacheDiskCacheSettings.RBM_KEYSTORE_SIZE_KEY; -import static org.opensearch.cache.EhcacheDiskCacheSettings.USE_RBM_KEYSTORE_KEY; +import static org.opensearch.cache.EhcacheDiskCacheSettings.KEYSTORE_SIZE_KEY; +import static org.opensearch.cache.EhcacheDiskCacheSettings.USE_KEYSTORE_KEY; /** * This variant of disk cache uses Ehcache underneath. @@ -147,14 +148,14 @@ private EhcacheDiskCache(Builder builder) { this.removalListener = builder.getRemovalListener(); this.ehCacheEventListener = new EhCacheEventListener(builder.getRemovalListener()); this.cache = buildCache(Duration.ofMillis(expireAfterAccess.getMillis()), builder); - boolean useRBMKeystore = (Boolean) EhcacheDiskCacheSettings.getSettingListForCacheType(cacheType) - .get(USE_RBM_KEYSTORE_KEY) - .get(settings); - if (useRBMKeystore) { + String keystoreType = (String) EhcacheDiskCacheSettings.getSettingListForCacheType(cacheType).get(USE_KEYSTORE_KEY).get(settings); + if (keystoreType.equals(RBMIntKeyLookupStore.KEYSTORE_NAME)) { long keystoreSize = ((ByteSizeValue) EhcacheDiskCacheSettings.getSettingListForCacheType(cacheType) - .get(RBM_KEYSTORE_SIZE_KEY) + .get(KEYSTORE_SIZE_KEY) .get(settings)).getBytes(); this.keystore = new RBMIntKeyLookupStore(keystoreSize); + } else { + this.keystore = new DummyKeystore(); } } @@ -253,7 +254,7 @@ public V get(K key) { throw new IllegalArgumentException("Key passed to ehcache disk cache was null."); } V value = null; - if (keystore == null || keystore.contains(key.hashCode()) || keystore.isFull()) { + if (keystore.contains(key.hashCode()) || keystore.isFull()) { try { value = cache.get(key); } catch (CacheLoadingException ex) { @@ -272,9 +273,7 @@ public V get(K key) { public void put(K key, V value) { try { cache.put(key, value); - if (keystore != null) { - keystore.add(key.hashCode()); - } + keystore.add(key.hashCode()); } catch (CacheWritingException ex) { throw new OpenSearchException("Exception occurred while put item to ehcache disk cache"); } @@ -404,6 +403,14 @@ public void close() { } } + /** + * Returns keystore, for testing. + * @return keystore + */ + KeyLookupStore getKeystore() { + return keystore; + } + /** * This iterator wraps ehCache iterator and only iterates over its keys. * @param Type of key 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 d5f5fbb9293bc..bd151e1ff9a93 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 @@ -9,6 +9,8 @@ package org.opensearch.cache.store.disk; import org.opensearch.cache.EhcacheDiskCacheSettings; +import org.opensearch.cache.keystore.DummyKeystore; +import org.opensearch.cache.keystore.RBMIntKeyLookupStore; import org.opensearch.common.cache.CacheType; import org.opensearch.common.cache.ICache; import org.opensearch.common.cache.LoadAwareCacheLoader; @@ -35,6 +37,7 @@ 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_STORAGE_PATH_KEY; +import static org.opensearch.cache.EhcacheDiskCacheSettings.USE_KEYSTORE_KEY; import static org.hamcrest.CoreMatchers.instanceOf; public class EhCacheDiskCacheTests extends OpenSearchSingleNodeTestCase { @@ -481,6 +484,97 @@ public String load(String key) throws Exception { } } + public void testKeystoreSettings() throws Exception { + Settings useRBMsettings = Settings.builder() + .put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE).get(USE_KEYSTORE_KEY).getKey(), + RBMIntKeyLookupStore.KEYSTORE_NAME + ) + .build(); + + MockRemovalListener removalListener = new MockRemovalListener<>(); + try (NodeEnvironment env = newNodeEnvironment(Settings.EMPTY)) { + EhcacheDiskCache ehcacheTest = (EhcacheDiskCache) new EhcacheDiskCache.Builder() + .setDiskCacheAlias("test1") + .setThreadPoolAlias("ehcacheTest") + .setIsEventListenerModeSync(true) + .setStoragePath(env.nodePaths()[0].indicesPath.toString() + "/request_cache") + .setKeyType(String.class) + .setValueType(String.class) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(useRBMsettings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .build(); + assertEquals(RBMIntKeyLookupStore.class, ehcacheTest.getKeystore().getClass()); + ehcacheTest.close(); + } + + Settings useDummySettings = Settings.builder() + .put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE).get(USE_KEYSTORE_KEY).getKey(), + "unrecognized_name" + ) + .build(); + + try (NodeEnvironment env = newNodeEnvironment(Settings.EMPTY)) { + EhcacheDiskCache ehcacheTest = (EhcacheDiskCache) new EhcacheDiskCache.Builder() + .setDiskCacheAlias("test1") + .setThreadPoolAlias("ehcacheTest") + .setIsEventListenerModeSync(true) + .setStoragePath(env.nodePaths()[0].indicesPath.toString() + "/request_cache") + .setKeyType(String.class) + .setValueType(String.class) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(useDummySettings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .build(); + assertEquals(DummyKeystore.class, ehcacheTest.getKeystore().getClass()); + + // Also test the dummy keystore doesn't incorrectly block any gets + int numKeys = 50; + Map keyValueMap = new HashMap<>(); + for (int i = 0; i < numKeys; i++) { + String key = generateRandomString(50); + String value = generateRandomString(50); + keyValueMap.put(key, value); + ehcacheTest.put(key, value); + } + for (String key : keyValueMap.keySet()) { + assertEquals(keyValueMap.get(key), ehcacheTest.get(key)); + } + ehcacheTest.close(); + } + + // Check the factory correctly gives RBM keystore + try (NodeEnvironment env = newNodeEnvironment(Settings.EMPTY)) { + ICache.Factory ehcacheFactory = new EhcacheDiskCache.EhcacheDiskCacheFactory(); + EhcacheDiskCache ehcacheTest = (EhcacheDiskCache) ehcacheFactory.create( + new CacheConfig.Builder().setValueType(String.class) + .setKeyType(String.class) + .setRemovalListener(removalListener) + .setSettings( + Settings.builder() + .put( + EhcacheDiskCacheSettings.getSettingListForCacheType(CacheType.INDICES_REQUEST_CACHE) + .get(DISK_STORAGE_PATH_KEY) + .getKey(), + env.nodePaths()[0].indicesPath.toString() + "/request_cache" + ) + .build() + ) + .build(), + CacheType.INDICES_REQUEST_CACHE, + Map.of() + ); + assertEquals(RBMIntKeyLookupStore.class, ehcacheTest.getKeystore().getClass()); + ehcacheTest.close(); + } + } + private static String generateRandomString(int length) { String characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; StringBuilder randomString = new StringBuilder(length);