diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index d65a20b033f52..7fc4fcbdc5cec 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -240,31 +240,19 @@ BytesReference getOrCompute( final Key key = new Key(((IndexShard) cacheEntity.getCacheIdentity()).shardId(), cacheKey, readerCacheKeyId); Loader cacheLoader = new Loader(cacheEntity, loader); BytesReference value = cache.computeIfAbsent(key, cacheLoader); - CleanupKey cleanupKey = null; - try { - if (cacheLoader.isLoaded()) { - cacheEntity.onMiss(); - // see if it's the first time we see this reader, and make sure to register a cleanup key - cleanupKey = new CleanupKey(cacheEntity, readerCacheKeyId); - if (!registeredClosedListeners.containsKey(cleanupKey)) { - Boolean previous = registeredClosedListeners.putIfAbsent(cleanupKey, Boolean.TRUE); - if (previous == null) { - OpenSearchDirectoryReader.addReaderCloseListener(reader, cleanupKey); - } + if (cacheLoader.isLoaded()) { + cacheEntity.onMiss(); + // see if it's the first time we see this reader, and make sure to register a cleanup key + CleanupKey cleanupKey = new CleanupKey(cacheEntity, readerCacheKeyId); + if (!registeredClosedListeners.containsKey(cleanupKey)) { + Boolean previous = registeredClosedListeners.putIfAbsent(cleanupKey, Boolean.TRUE); + if (previous == null) { + OpenSearchDirectoryReader.addReaderCloseListener(reader, cleanupKey); } - cacheCleanupManager.updateCleanupKeyToCountMapOnCacheInsertion(cleanupKey); - } else { - cacheEntity.onHit(); } - } catch (Exception e) { - if (logger.isDebugEnabled()) { - logger.debug("cleanupKey in Indices Request Cache failed to register close listener due to : " + e.getCause()); - } - // On failing to register the cleanupkey, this cache entry is immediately stale and could live indefinitely in the cache - // hence enqueuing it for clean-up cacheCleanupManager.updateCleanupKeyToCountMapOnCacheInsertion(cleanupKey); - cacheCleanupManager.enqueueCleanupKey(cleanupKey); - throw e; + } else { + cacheEntity.onHit(); } return value; } diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index faa03cd6706ad..be9cc1ea7905a 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -42,7 +42,6 @@ import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.TermQuery; import org.apache.lucene.search.TopDocs; -import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; import org.opensearch.common.CheckedSupplier; @@ -83,9 +82,7 @@ import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; -import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_CLEAN_INTERVAL_SETTING; import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING; -import static org.hamcrest.Matchers.instanceOf; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -655,39 +652,6 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { assertEquals(0, cleanupKeyToCountMap.size()); } - // test the cleanupKeyToCountMap are set appropriately in a case where the reader gets closed - // after putting an entry into the cache and before registering a close listener - // in this case, the entry is stale and needs to be accounted right and cleaned up accordingly - public void testCleanupKeyToOnClosedReader_cleansupStalenessAsExpected() throws Exception { - threadPool = getThreadPool(); - Settings settings = Settings.builder() - .put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51") - .put(INDICES_REQUEST_CACHE_CLEAN_INTERVAL_SETTING.getKey(), "10m") // intentionally high - .build(); - cache = getIndicesRequestCache(settings); - - writer.addDocument(newDoc(0, "foo")); - ShardId shardId = indexShard.shardId(); - DirectoryReader reader = getReader(writer, shardId); - - assertTrue(cache.cacheCleanupManager.getCleanupKeyToCountMap().isEmpty()); - try { - // use auto close loader to close the reader right after adding cache entry - cache.getOrCompute(getEntity(indexShard), getAutoCloseLoader(reader), reader, getTermBytes()); - } catch (Exception e) { - assertThat(e, instanceOf(AlreadyClosedException.class)); - } - assertEquals(1, cache.count()); - assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); - assertTrue(cache.cacheCleanupManager.getCleanupKeyToCountMap().isEmpty()); - - cache.cacheCleanupManager.cleanCache(); - - assertEquals(0, cache.count()); - assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); - assertTrue(cache.cacheCleanupManager.getCleanupKeyToCountMap().isEmpty()); - } - private DirectoryReader getReader(IndexWriter writer, ShardId shardId) throws IOException { return OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); } @@ -709,10 +673,6 @@ private Loader getLoader(DirectoryReader reader) { return new Loader(reader, 0); } - private AutoCloseLoader getAutoCloseLoader(DirectoryReader reader) { - return new AutoCloseLoader(reader, 0); - } - private IndicesService.IndexShardCacheEntity getEntity(IndexShard indexShard) { return new IndicesService.IndexShardCacheEntity(indexShard); } @@ -852,32 +812,6 @@ public BytesReference get() { } } - /* - * This class does everything Loader class does except closing the reader after get() - * This can be used for testing behaviours on closed readers. - * */ - private static class AutoCloseLoader extends Loader { - - AutoCloseLoader(DirectoryReader reader, int id) { - super(reader, id); - } - - @Override - public BytesReference get() { - BytesReference output = super.get(); - try { - closeReader(); - } catch (IOException e) { - throw new RuntimeException(e); - } - return output; - } - - private void closeReader() throws IOException { - reader.close(); - } - } - public void testInvalidate() throws Exception { threadPool = getThreadPool(); IndicesRequestCache cache = getIndicesRequestCache(Settings.EMPTY);