From 80ef05629af688119167ab1f3b41e00c1784cdee Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Thu, 11 Jul 2024 16:36:40 -0700 Subject: [PATCH 1/9] Clear ehcache disk cache files during initialization Signed-off-by: Sagar Upadhyaya --- .../cache/store/disk/EhcacheDiskCache.java | 44 +++++++++++++--- .../store/disk/EhCacheDiskCacheTests.java | 52 +++++++++++++++++++ 2 files changed, 88 insertions(+), 8 deletions(-) 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 b4c62fbf85cb8..294896ff07f28 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 @@ -62,6 +62,7 @@ import org.ehcache.Cache; import org.ehcache.CachePersistenceException; import org.ehcache.PersistentCacheManager; +import org.ehcache.StateTransitionException; import org.ehcache.config.builders.CacheConfigurationBuilder; import org.ehcache.config.builders.CacheEventListenerConfigurationBuilder; import org.ehcache.config.builders.CacheManagerBuilder; @@ -133,6 +134,7 @@ public class EhcacheDiskCache implements ICache { */ Map, CompletableFuture, V>>> completableFutureMap = new ConcurrentHashMap<>(); + @SuppressForbidden(reason = "Ehcache uses File.io") private EhcacheDiskCache(Builder builder) { this.keyType = Objects.requireNonNull(builder.keyType, "Key type shouldn't be null"); this.valueType = Objects.requireNonNull(builder.valueType, "Value type shouldn't be null"); @@ -151,6 +153,23 @@ private EhcacheDiskCache(Builder builder) { if (this.storagePath == null || this.storagePath.isBlank()) { throw new IllegalArgumentException("Storage path shouldn't be null or empty"); } + // Delete all the previous disk cache related files/data. We don't persist data between process restart for + // now which is why need to do this. Clean up in case there was a non graceful restart and we had older disk + // cache data still lying around. + Path ehcacheDirectory = Paths.get(this.storagePath); + if (Files.exists(ehcacheDirectory)) { + try { + logger.info("Found older disk cache data lying around during initialization under path: {}", this.storagePath); + IOUtils.rm(ehcacheDirectory); + } catch (IOException e) { + logger.error( + () -> new ParameterizedMessage( + "Failed to delete ehcache disk cache data under path: {} " + "during initial", + this.storagePath + ) + ); + } + } if (builder.threadPoolAlias == null || builder.threadPoolAlias.isBlank()) { this.threadPoolAlias = THREAD_POOL_ALIAS_PREFIX + "DiskWrite#" + UNIQUE_ID; } else { @@ -444,19 +463,28 @@ public void refresh() { @Override @SuppressForbidden(reason = "Ehcache uses File.io") public void close() { - cacheManager.removeCache(this.diskCacheAlias); - cacheManager.close(); try { + cacheManager.removeCache(this.diskCacheAlias); + cacheManager.close(); cacheManager.destroyCache(this.diskCacheAlias); - // Delete all the disk cache related files/data + } catch (CachePersistenceException e) { + // When something goes wrong while destroying the persistence data + logger.error(() -> new ParameterizedMessage("Exception occurred while destroying ehcache and associated " + "data"), e); + throw new OpenSearchException("Exception occurred while destroying ehcache and associated data", e); + } catch (StateTransitionException e) { + logger.error(() -> new ParameterizedMessage("Exception occurred while trying to close/remove ehcache"), e); + } finally { + // Delete all the disk cache related files/data in case it is present Path ehcacheDirectory = Paths.get(this.storagePath); if (Files.exists(ehcacheDirectory)) { - IOUtils.rm(ehcacheDirectory); + try { + IOUtils.rm(ehcacheDirectory); + } catch (IOException e) { + logger.error( + () -> new ParameterizedMessage("Failed to delete ehcache disk cache data under path: {}", this.storagePath) + ); + } } - } catch (CachePersistenceException e) { - throw new OpenSearchException("Exception occurred while destroying ehcache and associated data", e); - } catch (IOException e) { - logger.error(() -> new ParameterizedMessage("Failed to delete ehcache disk cache data under path: {}", this.storagePath)); } } 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 29551befd3e9f..3f358e997aa64 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 @@ -34,6 +34,8 @@ import java.io.IOException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.ArrayList; import java.util.HashMap; import java.util.Iterator; @@ -882,6 +884,56 @@ public void testStatsTrackingDisabled() throws Exception { } } + public void testDiskCacheFilesAreClearedUpDuringCloseAndInitialization() throws Exception { + Settings settings = Settings.builder().build(); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + String path = env.nodePaths()[0].path.toString() + "/request_cache"; + // Create a dummy file to simulate a scenario where the data is already in the disk cache storage path + // beforehand. + Files.createDirectory(Path.of(path)); + Path dummyFilePath = Files.createFile(Path.of(path + "/testing.txt")); + assertTrue(Files.exists(dummyFilePath)); + ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") + .setStoragePath(path) + .setIsEventListenerModeSync(true) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setDiskCacheAlias("test1") + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .setStatsTrackingEnabled(false) + .build(); + int randomKeys = randomIntBetween(10, 100); + for (int i = 0; i < randomKeys; i++) { + ICacheKey iCacheKey = getICacheKey(UUID.randomUUID().toString()); + ehcacheTest.put(iCacheKey, UUID.randomUUID().toString()); + assertEquals(0, ehcacheTest.count()); // Expect count of 0 if NoopCacheStatsHolder is used + assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), ehcacheTest.stats().getTotalStats()); + } + // Verify that older data was wiped out after initialization + assertFalse(Files.exists(dummyFilePath)); + + // Verify that there is data present under desired path by explicitly verifying the folder name by prefix + // (used from disk cache alias) + assertTrue(Files.exists(Path.of(path))); + boolean folderExists = Files.walk(Path.of(path)) + .filter(Files::isDirectory) + .anyMatch(path1 -> path1.getFileName().toString().startsWith("test1")); + assertTrue(folderExists); + ehcacheTest.close(); + assertFalse(Files.exists(Path.of(path))); // Verify everything is cleared up now after close() + } + } + private List getRandomDimensions(List dimensionNames) { Random rand = Randomness.get(); int bound = 3; From 54220643e30c91cbd620ef198274294667352495 Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Thu, 11 Jul 2024 22:03:59 -0700 Subject: [PATCH 2/9] Adding UT to fix line coverage Signed-off-by: Sagar Upadhyaya --- .../cache/store/disk/EhcacheDiskCache.java | 8 +--- .../store/disk/EhCacheDiskCacheTests.java | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) 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 294896ff07f28..c55c0543486aa 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 @@ -60,9 +60,7 @@ import java.util.function.ToLongBiFunction; import org.ehcache.Cache; -import org.ehcache.CachePersistenceException; import org.ehcache.PersistentCacheManager; -import org.ehcache.StateTransitionException; import org.ehcache.config.builders.CacheConfigurationBuilder; import org.ehcache.config.builders.CacheEventListenerConfigurationBuilder; import org.ehcache.config.builders.CacheManagerBuilder; @@ -467,11 +465,7 @@ public void close() { cacheManager.removeCache(this.diskCacheAlias); cacheManager.close(); cacheManager.destroyCache(this.diskCacheAlias); - } catch (CachePersistenceException e) { - // When something goes wrong while destroying the persistence data - logger.error(() -> new ParameterizedMessage("Exception occurred while destroying ehcache and associated " + "data"), e); - throw new OpenSearchException("Exception occurred while destroying ehcache and associated data", e); - } catch (StateTransitionException e) { + } catch (Exception e) { logger.error(() -> new ParameterizedMessage("Exception occurred while trying to close/remove ehcache"), e); } finally { // Delete all the disk cache related files/data in case it is present 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 3f358e997aa64..a722437954f29 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 @@ -934,6 +934,43 @@ public void testDiskCacheFilesAreClearedUpDuringCloseAndInitialization() throws } } + public void testDiskCacheCloseCalledTwiceAndVerifyDiskDataIsCleanedUp() throws Exception { + Settings settings = Settings.builder().build(); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + String path = env.nodePaths()[0].path.toString() + "/request_cache"; + ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") + .setStoragePath(path) + .setIsEventListenerModeSync(true) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setDiskCacheAlias("test1") + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .setStatsTrackingEnabled(false) + .build(); + int randomKeys = randomIntBetween(10, 100); + for (int i = 0; i < randomKeys; i++) { + ICacheKey iCacheKey = getICacheKey(UUID.randomUUID().toString()); + ehcacheTest.put(iCacheKey, UUID.randomUUID().toString()); + assertEquals(0, ehcacheTest.count()); // Expect count storagePath 0 if NoopCacheStatsHolder is used + assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), ehcacheTest.stats().getTotalStats()); + } + ehcacheTest.close(); + // Call it again. This will throw an exception. + ehcacheTest.close(); + assertFalse(Files.exists(Path.of(path))); // Verify everything is cleared up now after close() + } + } + private List getRandomDimensions(List dimensionNames) { Random rand = Randomness.get(); int bound = 3; From 70b243e2186d047cf7b2e857a25453919186cfdc Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Thu, 11 Jul 2024 22:26:22 -0700 Subject: [PATCH 3/9] Addressing comment Signed-off-by: Sagar Upadhyaya --- .../org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a722437954f29..dbe58e823cf3c 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 @@ -965,9 +965,9 @@ public void testDiskCacheCloseCalledTwiceAndVerifyDiskDataIsCleanedUp() throws E assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), ehcacheTest.stats().getTotalStats()); } ehcacheTest.close(); + assertFalse(Files.exists(Path.of(path))); // Verify everything is cleared up now after close() // Call it again. This will throw an exception. ehcacheTest.close(); - assertFalse(Files.exists(Path.of(path))); // Verify everything is cleared up now after close() } } From 3b36f497c439efe266d4e6f0600ce613b5ea3b4f Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Fri, 12 Jul 2024 11:33:13 -0700 Subject: [PATCH 4/9] Adding more Uts for better line coverage Signed-off-by: Sagar Upadhyaya --- .../cache/store/disk/EhcacheDiskCache.java | 2 +- .../store/disk/EhCacheDiskCacheTests.java | 56 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) 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 c55c0543486aa..02952759f5ded 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 @@ -103,7 +103,7 @@ public class EhcacheDiskCache implements ICache { // Unique id associated with this cache. private final static String UNIQUE_ID = UUID.randomUUID().toString(); private final static String THREAD_POOL_ALIAS_PREFIX = "ehcachePool"; - private final static int MINIMUM_MAX_SIZE_IN_BYTES = 1024 * 100; // 100KB + final static int MINIMUM_MAX_SIZE_IN_BYTES = 1024 * 100; // 100KB // A Cache manager can create many caches. private final PersistentCacheManager cacheManager; 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 dbe58e823cf3c..e8f4fc623198a 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 @@ -52,6 +52,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.store.disk.EhcacheDiskCache.MINIMUM_MAX_SIZE_IN_BYTES; import static org.hamcrest.CoreMatchers.instanceOf; @ThreadLeakFilters(filters = { EhcacheThreadLeakFilter.class }) @@ -905,6 +906,7 @@ public void testDiskCacheFilesAreClearedUpDuringCloseAndInitialization() throws .setValueSerializer(new StringSerializer()) .setDimensionNames(List.of(dimensionName)) .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setThreadPoolAlias("") .setSettings(settings) .setExpireAfterAccess(TimeValue.MAX_VALUE) .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) @@ -971,6 +973,60 @@ public void testDiskCacheCloseCalledTwiceAndVerifyDiskDataIsCleanedUp() throws E } } + public void testEhcacheDiskCacheWithoutStoragePathDefined() throws Exception { + Settings settings = Settings.builder().build(); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + assertThrows( + IllegalArgumentException.class, + () -> new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") + .setIsEventListenerModeSync(true) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setDiskCacheAlias("test1") + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .setStatsTrackingEnabled(false) + .build() + ); + } + } + + public void testEhcacheWithStorageSizeLowerThanMinimumExpected() throws Exception { + Settings settings = Settings.builder().build(); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + assertThrows( + IllegalArgumentException.class, + () -> new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") + .setIsEventListenerModeSync(true) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setDiskCacheAlias("test1") + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(MINIMUM_MAX_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .setStatsTrackingEnabled(false) + .build() + ); + } + } + private List getRandomDimensions(List dimensionNames) { Random rand = Randomness.get(); int bound = 3; From 9f69b6fef94de43c57334a7348fdd4b1be9e77b2 Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Mon, 15 Jul 2024 14:47:57 -0700 Subject: [PATCH 5/9] Throwing exception in case we fail to clear cache files during startup Signed-off-by: Sagar Upadhyaya --- .../cache/store/disk/EhcacheDiskCache.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) 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 02952759f5ded..aa8c1601bd0cd 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 @@ -103,8 +103,6 @@ public class EhcacheDiskCache implements ICache { // Unique id associated with this cache. private final static String UNIQUE_ID = UUID.randomUUID().toString(); private final static String THREAD_POOL_ALIAS_PREFIX = "ehcachePool"; - final static int MINIMUM_MAX_SIZE_IN_BYTES = 1024 * 100; // 100KB - // A Cache manager can create many caches. private final PersistentCacheManager cacheManager; @@ -126,6 +124,10 @@ public class EhcacheDiskCache implements ICache { private final Serializer keySerializer; private final Serializer valueSerializer; + final static int MINIMUM_MAX_SIZE_IN_BYTES = 1024 * 100; // 100KB + final static String CACHE_DATA_CLEANUP_DURING_INITIALIZATION_EXCEPTION = "Failed to delete ehcache disk cache under " + + "path: % during initialization. Please clean this up manually and restart the process"; + /** * Used in computeIfAbsent to synchronize loading of a given key. This is needed as ehcache doesn't provide a * computeIfAbsent method. @@ -160,12 +162,7 @@ private EhcacheDiskCache(Builder builder) { logger.info("Found older disk cache data lying around during initialization under path: {}", this.storagePath); IOUtils.rm(ehcacheDirectory); } catch (IOException e) { - logger.error( - () -> new ParameterizedMessage( - "Failed to delete ehcache disk cache data under path: {} " + "during initial", - this.storagePath - ) - ); + throw new OpenSearchException(String.format(CACHE_DATA_CLEANUP_DURING_INITIALIZATION_EXCEPTION, this.storagePath)); } } if (builder.threadPoolAlias == null || builder.threadPoolAlias.isBlank()) { From a611976e901157962923ffb64c7c319a21eb23e0 Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Tue, 16 Jul 2024 10:35:54 -0700 Subject: [PATCH 6/9] Adding more UTs Signed-off-by: Sagar Upadhyaya --- .../cache/store/disk/EhcacheDiskCache.java | 31 ++++--- .../store/disk/EhCacheDiskCacheTests.java | 93 ++++++++++++++++++- 2 files changed, 111 insertions(+), 13 deletions(-) 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 aa8c1601bd0cd..8875c0617d652 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 @@ -458,23 +458,30 @@ public void refresh() { @Override @SuppressForbidden(reason = "Ehcache uses File.io") public void close() { + // We are calling each of below function in its own try/catch block so that we still try to close the cache + // manager in case removeCache failed (for example) try { cacheManager.removeCache(this.diskCacheAlias); + } catch (Exception e) { + logger.error(() -> new ParameterizedMessage("Exception occurred while trying to remove cache from manager"), e); + } + try { cacheManager.close(); + } catch (Exception e) { + logger.error(() -> new ParameterizedMessage("Exception occurred while trying to close ehcache manager"), e); + } + try { cacheManager.destroyCache(this.diskCacheAlias); } catch (Exception e) { - logger.error(() -> new ParameterizedMessage("Exception occurred while trying to close/remove ehcache"), e); - } finally { - // Delete all the disk cache related files/data in case it is present - Path ehcacheDirectory = Paths.get(this.storagePath); - if (Files.exists(ehcacheDirectory)) { - try { - IOUtils.rm(ehcacheDirectory); - } catch (IOException e) { - logger.error( - () -> new ParameterizedMessage("Failed to delete ehcache disk cache data under path: {}", this.storagePath) - ); - } + logger.error(() -> new ParameterizedMessage("Exception occurred while trying to destroy cache and " + "associated data"), e); + } + // Delete all the disk cache related files/data in case it is present + Path ehcacheDirectory = Paths.get(this.storagePath); + if (Files.exists(ehcacheDirectory)) { + try { + IOUtils.rm(ehcacheDirectory); + } catch (IOException e) { + logger.error(() -> new ParameterizedMessage("Failed to delete ehcache disk cache data under path: {}", this.storagePath)); } } } 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 e8f4fc623198a..1bc30e295ab97 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 @@ -25,6 +25,7 @@ import org.opensearch.common.metrics.CounterMetric; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.io.IOUtils; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.common.bytes.CompositeBytesReference; @@ -942,7 +943,7 @@ public void testDiskCacheCloseCalledTwiceAndVerifyDiskDataIsCleanedUp() throws E ToLongBiFunction, String> weigher = getWeigher(); try (NodeEnvironment env = newNodeEnvironment(settings)) { String path = env.nodePaths()[0].path.toString() + "/request_cache"; - ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") + ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias(null) .setStoragePath(path) .setIsEventListenerModeSync(true) .setKeyType(String.class) @@ -973,6 +974,41 @@ public void testDiskCacheCloseCalledTwiceAndVerifyDiskDataIsCleanedUp() throws E } } + public void testDiskCacheCloseAfterCleaningUpFilesManually() throws Exception { + Settings settings = Settings.builder().build(); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + String path = env.nodePaths()[0].path.toString() + "/request_cache"; + ICache ehcacheTest = new EhcacheDiskCache.Builder().setThreadPoolAlias(null) + .setStoragePath(path) + .setIsEventListenerModeSync(true) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setDiskCacheAlias("test1") + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .setStatsTrackingEnabled(false) + .build(); + int randomKeys = randomIntBetween(10, 100); + for (int i = 0; i < randomKeys; i++) { + ICacheKey iCacheKey = getICacheKey(UUID.randomUUID().toString()); + ehcacheTest.put(iCacheKey, UUID.randomUUID().toString()); + assertEquals(0, ehcacheTest.count()); // Expect count storagePath 0 if NoopCacheStatsHolder is used + assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), ehcacheTest.stats().getTotalStats()); + } + IOUtils.rm(Path.of(path)); + ehcacheTest.close(); + } + } + public void testEhcacheDiskCacheWithoutStoragePathDefined() throws Exception { Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); @@ -1000,6 +1036,34 @@ public void testEhcacheDiskCacheWithoutStoragePathDefined() throws Exception { } } + public void testEhcacheDiskCacheWithoutStoragePathNull() throws Exception { + Settings settings = Settings.builder().build(); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + assertThrows( + IllegalArgumentException.class, + () -> new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") + .setStoragePath(null) + .setIsEventListenerModeSync(true) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setDiskCacheAlias("test1") + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .setStatsTrackingEnabled(false) + .build() + ); + } + } + public void testEhcacheWithStorageSizeLowerThanMinimumExpected() throws Exception { Settings settings = Settings.builder().build(); MockRemovalListener removalListener = new MockRemovalListener<>(); @@ -1027,6 +1091,33 @@ public void testEhcacheWithStorageSizeLowerThanMinimumExpected() throws Exceptio } } + public void testEhcacheWithStorageSizeZero() throws Exception { + Settings settings = Settings.builder().build(); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + assertThrows( + IllegalArgumentException.class, + () -> new EhcacheDiskCache.Builder().setThreadPoolAlias("ehcacheTest") + .setIsEventListenerModeSync(true) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setDiskCacheAlias("test1") + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(0) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .setStatsTrackingEnabled(false) + .build() + ); + } + } + private List getRandomDimensions(List dimensionNames) { Random rand = Randomness.get(); int bound = 3; From 5e179dc568f24fe0c1127344f33f52963c58ab38 Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Tue, 16 Jul 2024 15:35:33 -0700 Subject: [PATCH 7/9] Adding a UT for more coverage Signed-off-by: Sagar Upadhyaya --- .../cache/store/disk/EhcacheDiskCache.java | 15 +++-- .../store/disk/EhCacheDiskCacheTests.java | 57 +++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) 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 8875c0617d652..8270acdaf0024 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 @@ -126,7 +126,7 @@ public class EhcacheDiskCache implements ICache { final static int MINIMUM_MAX_SIZE_IN_BYTES = 1024 * 100; // 100KB final static String CACHE_DATA_CLEANUP_DURING_INITIALIZATION_EXCEPTION = "Failed to delete ehcache disk cache under " - + "path: % during initialization. Please clean this up manually and restart the process"; + + "path: %s during initialization. Please clean this up manually and restart the process"; /** * Used in computeIfAbsent to synchronize loading of a given key. This is needed as ehcache doesn't provide a @@ -135,7 +135,7 @@ public class EhcacheDiskCache implements ICache { Map, CompletableFuture, V>>> completableFutureMap = new ConcurrentHashMap<>(); @SuppressForbidden(reason = "Ehcache uses File.io") - private EhcacheDiskCache(Builder builder) { + EhcacheDiskCache(Builder builder) { this.keyType = Objects.requireNonNull(builder.keyType, "Key type shouldn't be null"); this.valueType = Objects.requireNonNull(builder.valueType, "Value type shouldn't be null"); this.expireAfterAccess = Objects.requireNonNull(builder.getExpireAfterAcess(), "ExpireAfterAccess value shouldn't " + "be null"); @@ -162,7 +162,7 @@ private EhcacheDiskCache(Builder builder) { logger.info("Found older disk cache data lying around during initialization under path: {}", this.storagePath); IOUtils.rm(ehcacheDirectory); } catch (IOException e) { - throw new OpenSearchException(String.format(CACHE_DATA_CLEANUP_DURING_INITIALIZATION_EXCEPTION, this.storagePath)); + throw new OpenSearchException(String.format(CACHE_DATA_CLEANUP_DURING_INITIALIZATION_EXCEPTION, this.storagePath), e); } } if (builder.threadPoolAlias == null || builder.threadPoolAlias.isBlank()) { @@ -189,6 +189,11 @@ private EhcacheDiskCache(Builder builder) { } } + // Package private for testing + PersistentCacheManager getCacheManager() { + return this.cacheManager; + } + @SuppressWarnings({ "rawtypes" }) private Cache buildCache(Duration expireAfterAccess, Builder builder) { // Creating the cache requires permissions specified in plugin-security.policy @@ -269,7 +274,7 @@ Map, CompletableFuture, V>>> getCompletableFutur } @SuppressForbidden(reason = "Ehcache uses File.io") - private PersistentCacheManager buildCacheManager() { + PersistentCacheManager buildCacheManager() { // In case we use multiple ehCaches, we can define this cache manager at a global level. // Creating the cache manager also requires permissions specified in plugin-security.policy return AccessController.doPrivileged((PrivilegedAction) () -> { @@ -473,7 +478,7 @@ public void close() { try { cacheManager.destroyCache(this.diskCacheAlias); } catch (Exception e) { - logger.error(() -> new ParameterizedMessage("Exception occurred while trying to destroy cache and " + "associated data"), e); + logger.error(() -> new ParameterizedMessage("Exception occurred while trying to destroy cache and associated data"), e); } // Delete all the disk cache related files/data in case it is present Path ehcacheDirectory = Paths.get(this.storagePath); 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 1bc30e295ab97..073bec0710ba2 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 @@ -50,11 +50,17 @@ import java.util.concurrent.Phaser; import java.util.function.ToLongBiFunction; +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_STORAGE_PATH_KEY; import static org.opensearch.cache.store.disk.EhcacheDiskCache.MINIMUM_MAX_SIZE_IN_BYTES; import static org.hamcrest.CoreMatchers.instanceOf; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; @ThreadLeakFilters(filters = { EhcacheThreadLeakFilter.class }) public class EhCacheDiskCacheTests extends OpenSearchSingleNodeTestCase { @@ -1118,6 +1124,57 @@ public void testEhcacheWithStorageSizeZero() throws Exception { } } + public void testEhcacheCloseWithDestroyCacheMethodThrowingException() throws Exception { + EhcacheDiskCache ehcacheDiskCache = new MockEhcahceDiskCache(createDummyBuilder(null)); + PersistentCacheManager cacheManager = ehcacheDiskCache.getCacheManager(); + doNothing().when(cacheManager).removeCache(anyString()); + doNothing().when(cacheManager).close(); + doThrow(new RuntimeException("test")).when(cacheManager).destroyCache(anyString()); + ehcacheDiskCache.close(); + } + + class MockEhcahceDiskCache extends EhcacheDiskCache { + + public MockEhcahceDiskCache(Builder builder) { + super(builder); + } + + @Override + PersistentCacheManager buildCacheManager() { + PersistentCacheManager cacheManager = mock(PersistentCacheManager.class); + return cacheManager; + } + } + + private EhcacheDiskCache.Builder createDummyBuilder(String storagePath) throws IOException { + Settings settings = Settings.builder().build(); + MockRemovalListener removalListener = new MockRemovalListener<>(); + ToLongBiFunction, String> weigher = getWeigher(); + try (NodeEnvironment env = newNodeEnvironment(settings)) { + if (storagePath == null || storagePath.isBlank()) { + storagePath = env.nodePaths()[0].path.toString() + "/request_cache"; + } + return (EhcacheDiskCache.Builder) new EhcacheDiskCache.Builder().setThreadPoolAlias( + "ehcacheTest" + ) + .setIsEventListenerModeSync(true) + .setStoragePath(storagePath) + .setKeyType(String.class) + .setValueType(String.class) + .setKeySerializer(new StringSerializer()) + .setDiskCacheAlias("test1") + .setValueSerializer(new StringSerializer()) + .setDimensionNames(List.of(dimensionName)) + .setCacheType(CacheType.INDICES_REQUEST_CACHE) + .setSettings(settings) + .setExpireAfterAccess(TimeValue.MAX_VALUE) + .setMaximumWeightInBytes(CACHE_SIZE_IN_BYTES) + .setRemovalListener(removalListener) + .setWeigher(weigher) + .setStatsTrackingEnabled(false); + } + } + private List getRandomDimensions(List dimensionNames) { Random rand = Randomness.get(); int bound = 3; From 925bd8dd92a0891314e90012cec968638cb79892 Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Tue, 16 Jul 2024 15:57:23 -0700 Subject: [PATCH 8/9] Fixing gradle build Signed-off-by: Sagar Upadhyaya --- .../opensearch/cache/store/disk/EhCacheDiskCacheTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 073bec0710ba2..2bc24227bb513 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 @@ -1125,7 +1125,7 @@ public void testEhcacheWithStorageSizeZero() throws Exception { } public void testEhcacheCloseWithDestroyCacheMethodThrowingException() throws Exception { - EhcacheDiskCache ehcacheDiskCache = new MockEhcahceDiskCache(createDummyBuilder(null)); + EhcacheDiskCache ehcacheDiskCache = new MockEhcahceDiskCache(createDummyBuilder(null)); PersistentCacheManager cacheManager = ehcacheDiskCache.getCacheManager(); doNothing().when(cacheManager).removeCache(anyString()); doNothing().when(cacheManager).close(); @@ -1133,7 +1133,7 @@ public void testEhcacheCloseWithDestroyCacheMethodThrowingException() throws Exc ehcacheDiskCache.close(); } - class MockEhcahceDiskCache extends EhcacheDiskCache { + static class MockEhcahceDiskCache extends EhcacheDiskCache { public MockEhcahceDiskCache(Builder builder) { super(builder); From 4a666bb220adbf5f6996b97d6aeb330334cff716 Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Wed, 17 Jul 2024 11:06:19 -0700 Subject: [PATCH 9/9] Update ehcache disk cache close() logic Signed-off-by: Sagar Upadhyaya --- .../cache/store/disk/EhcacheDiskCache.java | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) 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 8270acdaf0024..4a95b04de3952 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 @@ -463,23 +463,11 @@ public void refresh() { @Override @SuppressForbidden(reason = "Ehcache uses File.io") public void close() { - // We are calling each of below function in its own try/catch block so that we still try to close the cache - // manager in case removeCache failed (for example) - try { - cacheManager.removeCache(this.diskCacheAlias); - } catch (Exception e) { - logger.error(() -> new ParameterizedMessage("Exception occurred while trying to remove cache from manager"), e); - } try { cacheManager.close(); } catch (Exception e) { logger.error(() -> new ParameterizedMessage("Exception occurred while trying to close ehcache manager"), e); } - try { - cacheManager.destroyCache(this.diskCacheAlias); - } catch (Exception e) { - logger.error(() -> new ParameterizedMessage("Exception occurred while trying to destroy cache and associated data"), e); - } // Delete all the disk cache related files/data in case it is present Path ehcacheDirectory = Paths.get(this.storagePath); if (Files.exists(ehcacheDirectory)) { @@ -489,6 +477,7 @@ public void close() { logger.error(() -> new ParameterizedMessage("Failed to delete ehcache disk cache data under path: {}", this.storagePath)); } } + } /**