From 2911c25f4a4cc4a1d02138918355efaecb51aba7 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Wed, 3 Apr 2024 14:26:44 -0700 Subject: [PATCH 01/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 108 +++++++++++++----- 1 file changed, 81 insertions(+), 27 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 34c8d6cf5e840..566258b33ad02 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -45,6 +45,7 @@ import org.opensearch.common.cache.LoadAwareCacheLoader; import org.opensearch.common.cache.RemovalListener; import org.opensearch.common.cache.RemovalNotification; +import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.policy.CachedQueryResult; import org.opensearch.common.cache.serializer.BytesReferenceSerializer; import org.opensearch.common.cache.service.CacheService; @@ -207,9 +208,20 @@ public void onRemoval(RemovalNotification notification) { // shards as part of request cache. Key key = notification.getKey(); cacheEntityLookup.apply(key.shardId).ifPresent(entity -> entity.onRemoval(notification)); - cacheCleanupManager.updateCleanupKeyToCountMapOnCacheEviction( - new CleanupKey(cacheEntityLookup.apply(key.shardId).orElse(null), key.readerCacheKeyId) - ); + if (notificationAffectsStaleness(notification)) { + CleanupKey cleanupKey = new CleanupKey(cacheEntityLookup.apply(key.shardId).orElse(null), key.readerCacheKeyId); + cacheCleanupManager.updateCleanupKeyToCountMapOnCacheEviction(cleanupKey); + } + } + + /** + * This method checks if the removal reason of the notification is not REPLACED. + * The reason of the notification is REPLACED when a cache entry's value is updated. + * If the removal reason is anything other than REPLACED, it will return true. + * If the removal reason is REPLACED, it will return false. + */ + private boolean notificationAffectsStaleness(RemovalNotification notification) { + return notification.getRemovalReason() != RemovalReason.REPLACED; } BytesReference getOrCompute( @@ -437,8 +449,23 @@ public int hashCode() { * If Staleness threshold is 0, we do not keep track of stale keys in the cache * */ class IndicesRequestCacheCleanupManager implements Closeable { + class StalenessProperty { + private int count; + private boolean isAccounted; + + // for testing + int getCount() { + return count; + } + + // for testing + boolean getIsAccounted() { + return isAccounted; + } + } + private final Set keysToClean; - private final ConcurrentMap> cleanupKeyToCountMap; + private final ConcurrentMap> cleanupKeyToCountMap; private final AtomicInteger staleKeysCount; private final double stalenessThreshold; private final IndicesRequestCacheCleaner cacheCleaner; @@ -487,9 +514,31 @@ private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { // If the key doesn't exist, it's added with a value of 1. // If the key exists, its value is incremented by 1. - cleanupKeyToCountMap.computeIfAbsent(shardId, k -> new HashMap<>()).merge(cleanupKey.readerCacheKeyId, 1, Integer::sum); + cleanupKeyToCountMap.compute(shardId, (currentShardId, stalenessPropertyMap) -> { + if (stalenessPropertyMap == null) { + stalenessPropertyMap = new HashMap<>(); + } + stalenessPropertyMap.compute(cleanupKey.readerCacheKeyId, (readerCacheKey, stalenessProperty) -> { + if (stalenessProperty == null) { + stalenessProperty = new StalenessProperty(); + } + stalenessProperty.count += 1; + return stalenessProperty; + }); + return stalenessPropertyMap; + }); } + /** + * Updates the cleanupKeyToCountMap and staleKeysCount when a cache eviction occurs. + * + *

This method is called when an entry is evicted from the cache. + * It decrements the count of the entry in the cleanupKeyToCountMap. + * It also decrements the staleKeysCount only if the entry was accounted. + * If the count of the CleanupKey becomes zero, it removes the CleanupKey from the map. + * + * @param cleanupKey the CleanupKey that has been evicted from the cache + */ private void updateCleanupKeyToCountMapOnCacheEviction(CleanupKey cleanupKey) { if (stalenessThreshold == 0.0 || cleanupKey.entity == null) { return; @@ -501,15 +550,17 @@ private void updateCleanupKeyToCountMapOnCacheEviction(CleanupKey cleanupKey) { } ShardId shardId = indexShard.shardId(); - cleanupKeyToCountMap.computeIfPresent(shardId, (shard, keyCountMap) -> { - keyCountMap.computeIfPresent(cleanupKey.readerCacheKeyId, (key, currentValue) -> { - // decrement the stale key count - staleKeysCount.decrementAndGet(); - int newValue = currentValue - 1; + cleanupKeyToCountMap.computeIfPresent(shardId, (shard, stalenessPropertyMap) -> { + stalenessPropertyMap.computeIfPresent(cleanupKey.readerCacheKeyId, (readerCacheKey, stalenessProperty) -> { + if (stalenessProperty.isAccounted) { + staleKeysCount.decrementAndGet(); + } + stalenessProperty.count -= 1; // Remove the key if the new value is zero by returning null; otherwise, update with the new value. - return newValue == 0 ? null : newValue; + return stalenessProperty.count == 0 ? null : stalenessProperty; }); - return keyCountMap; + // If stalenessPropertyMap is empty after removal, return null to remove the entry for shardId + return stalenessPropertyMap.isEmpty() ? null : stalenessPropertyMap; }); } @@ -517,7 +568,7 @@ private void updateCleanupKeyToCountMapOnCacheEviction(CleanupKey cleanupKey) { * Updates the count of stale keys in the cache. * This method is called when a CleanupKey is added to the keysToClean set. * - * It increments the staleKeysCount by the count of the CleanupKey in the cleanupKeyToCountMap. + *

It increments the staleKeysCount by the count of the CleanupKey in the cleanupKeyToCountMap. * If the CleanupKey's readerCacheKeyId is null or the CleanupKey's entity is not open, it increments the staleKeysCount * by the total count of keys associated with the CleanupKey's ShardId in the cleanupKeyToCountMap and removes the ShardId from the map. * @@ -535,27 +586,26 @@ private void incrementStaleKeysCount(CleanupKey cleanupKey) { ShardId shardId = indexShard.shardId(); // Using computeIfPresent to atomically operate on the countMap for a given shardId - cleanupKeyToCountMap.computeIfPresent(shardId, (key, countMap) -> { + cleanupKeyToCountMap.computeIfPresent(shardId, (currentShardId, stalenessPropertyMap) -> { if (cleanupKey.readerCacheKeyId == null) { // Aggregate and add to staleKeysCount atomically if readerCacheKeyId is null - int totalSum = countMap.values().stream().mapToInt(Integer::intValue).sum(); + int totalSum = stalenessPropertyMap.values().stream().mapToInt(stalenessProperty -> stalenessProperty.count).sum(); staleKeysCount.addAndGet(totalSum); - // Return null to automatically remove the mapping for shardId - return null; + // Set isAccounted to true for all StalenessProperty in the map + stalenessPropertyMap.values().forEach(stalenessProperty -> stalenessProperty.isAccounted = true); + // return the updated map + return stalenessPropertyMap; } else { // Update staleKeysCount based on specific readerCacheKeyId, then remove it from the countMap - countMap.computeIfPresent(cleanupKey.readerCacheKeyId, (k, v) -> { - staleKeysCount.addAndGet(v); - // Return null to remove the key after updating staleKeysCount - return null; + stalenessPropertyMap.computeIfPresent(cleanupKey.readerCacheKeyId, (readerCacheKey, stalenessProperty) -> { + staleKeysCount.addAndGet(stalenessProperty.count); + stalenessProperty.isAccounted = true; + // return the updated stalenessProperty + return stalenessProperty; }); - - // Check if countMap is empty after removal to decide if we need to remove the shardId entry - if (countMap.isEmpty()) { - return null; // Returning null removes the entry for shardId - } } - return countMap; // Return the modified countMap to keep the mapping + // Return the modified stalenessPropertyMap + return stalenessPropertyMap; }); } @@ -670,6 +720,10 @@ public void close() { this.cacheCleaner.close(); } + public ConcurrentMap> getCleanupKeyToCountMap() { + return cleanupKeyToCountMap; + } + private final class IndicesRequestCacheCleaner implements Runnable, Releasable { private final IndicesRequestCacheCleanupManager cacheCleanupManager; From 05610d32c1f59cd4715f74cedf222f3abc973acf Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Wed, 3 Apr 2024 14:27:12 -0700 Subject: [PATCH 02/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCacheTests.java | 340 ++++++++++++------ 1 file changed, 239 insertions(+), 101 deletions(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 594b9aac971b7..f99249e4dae00 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -76,8 +76,10 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.Optional; import java.util.UUID; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING; @@ -368,29 +370,17 @@ public void testCacheCleanupBasedOnZeroThreshold() throws Exception { DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - if (randomBoolean()) { - writer.flush(); - IOUtils.close(writer); - writer = new IndexWriter(dir, newIndexWriterConfig()); - } - writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); // Get 2 entries into the cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); cache.getOrCompute(entity, loader, reader, termBytes); - - entity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(1, cache.count()); IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); - cache.getOrCompute(entity, loader, secondReader, termBytes); - secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); cache.getOrCompute(secondEntity, loader, secondReader, termBytes); assertEquals(2, cache.count()); @@ -429,47 +419,37 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessEqualToThreshold() th DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - if (randomBoolean()) { - writer.flush(); - IOUtils.close(writer); - writer = new IndexWriter(dir, newIndexWriterConfig()); - } - writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); // Get 2 entries into the cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); cache.getOrCompute(entity, loader, reader, termBytes); - - entity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(1, cache.count()); IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); - cache.getOrCompute(entity, loader, secondReader, termBytes); - - secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); cache.getOrCompute(secondEntity, loader, secondReader, termBytes); assertEquals(2, cache.count()); // Close the reader, to be enqueued for cleanup - // 1 out of 2 keys ie 50% are now stale. reader.close(); + // 1 out of 2 keys ie 50% are now stale. + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); // cache count should not be affected assertEquals(2, cache.count()); // clean cache with 50% staleness threshold cache.cacheCleanupManager.cleanCache(); // cleanup should have taken effect + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); assertEquals(1, cache.count()); IOUtils.close(secondReader, writer, dir, cache); terminate(threadPool); } + // when a cache entry that is Stale is evicted for any reason, we have to deduct the count from our staleness count public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount() throws Exception { IndicesService indicesService = getInstanceFromNode(IndicesService.class); IndexShard indexShard = createIndex("test").getShard(0); @@ -486,63 +466,49 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); Directory dir = newDirectory(); IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + ShardId shardId = new ShardId("foo", "bar", 1); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - if (randomBoolean()) { - writer.flush(); - IOUtils.close(writer); - writer = new IndexWriter(dir, newIndexWriterConfig()); - } - writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); - // Get 2 entries into the cache + // Get 2 entries into the cache from 2 different readers IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); cache.getOrCompute(entity, loader, reader, termBytes); - - entity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(1, cache.count()); IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); - cache.getOrCompute(entity, loader, secondReader, termBytes); - - secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); cache.getOrCompute(secondEntity, loader, secondReader, termBytes); assertEquals(2, cache.count()); - // Close the reader, to be enqueued for cleanup + // assert no stale keys are accounted so far + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + // Close the reader, this should create a stale key reader.close(); - AtomicInteger staleKeysCount = cache.cacheCleanupManager.getStaleKeysCount(); // 1 out of 2 keys ie 50% are now stale. - assertEquals(1, staleKeysCount.get()); + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); // cache count should not be affected assertEquals(2, cache.count()); - OpenSearchDirectoryReader.DelegatingCacheHelper delegatingCacheHelper = - (OpenSearchDirectoryReader.DelegatingCacheHelper) secondReader.getReaderCacheHelper(); - String readerCacheKeyId = delegatingCacheHelper.getDelegatingCacheKey().getId(); IndicesRequestCache.Key key = new IndicesRequestCache.Key( ((IndexShard) secondEntity.getCacheIdentity()).shardId(), termBytes, - readerCacheKeyId + getReaderCacheKeyId(reader) ); cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.EVICTED)); - staleKeysCount = cache.cacheCleanupManager.getStaleKeysCount(); // eviction of previous stale key from the cache should decrement staleKeysCount in iRC - assertEquals(0, staleKeysCount.get()); + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); IOUtils.close(secondReader, writer, dir, cache); terminate(threadPool); } + // when a cache entry that is NOT Stale is evicted for any reason, we should NOT deduct it from staleness count public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStaleCount() throws Exception { IndicesService indicesService = getInstanceFromNode(IndicesService.class); IndexShard indexShard = createIndex("test").getShard(0); @@ -564,29 +530,16 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStal DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - if (randomBoolean()) { - writer.flush(); - IOUtils.close(writer); - writer = new IndexWriter(dir, newIndexWriterConfig()); - } - writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); // Get 2 entries into the cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); cache.getOrCompute(entity, loader, reader, termBytes); - - entity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(1, cache.count()); IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); - cache.getOrCompute(entity, loader, secondReader, termBytes); - - secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); cache.getOrCompute(secondEntity, loader, secondReader, termBytes); assertEquals(2, cache.count()); @@ -598,9 +551,8 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStal // cache count should not be affected assertEquals(2, cache.count()); - OpenSearchDirectoryReader.DelegatingCacheHelper delegatingCacheHelper = (OpenSearchDirectoryReader.DelegatingCacheHelper) reader - .getReaderCacheHelper(); - String readerCacheKeyId = delegatingCacheHelper.getDelegatingCacheKey().getId(); + // evict entry from second reader (this reader is not closed) + String readerCacheKeyId = getReaderCacheKeyId(secondReader); IndicesRequestCache.Key key = new IndicesRequestCache.Key( ((IndexShard) secondEntity.getCacheIdentity()).shardId(), termBytes, @@ -616,6 +568,55 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStal terminate(threadPool); } + // when a cache entry that is NOT Stale is evicted WITHOUT its reader closing, we should NOT deduct it from staleness count + public void testStaleCount_WithoutReaderClosing_OnRemovalNotificationOfStaleKey_DecrementsStaleCount() throws Exception { + IndicesService indicesService = getInstanceFromNode(IndicesService.class); + IndexShard indexShard = createIndex("test").getShard(0); + ThreadPool threadPool = getThreadPool(); + Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); + IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { + IndexService indexService = null; + try { + indexService = indicesService.indexServiceSafe(shardId.getIndex()); + } catch (IndexNotFoundException ex) { + return Optional.empty(); + } + return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); + }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); + Directory dir = newDirectory(); + IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + + writer.addDocument(newDoc(0, "foo")); + DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); + BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + + // Get 2 entries into the cache from 2 different readers + IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); + Loader loader = new Loader(reader, 0); + cache.getOrCompute(entity, loader, reader, termBytes); + + IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); + loader = new Loader(secondReader, 0); + cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + assertEquals(2, cache.count()); + // no keys are stale + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + // create notification for removal of non-stale entry + IndicesRequestCache.Key key = new IndicesRequestCache.Key( + ((IndexShard) secondEntity.getCacheIdentity()).shardId(), + termBytes, + getReaderCacheKeyId(reader) + ); + cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.EVICTED)); + // stale keys count should stay zero + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + + IOUtils.close(reader, secondReader, writer, dir, cache); + terminate(threadPool); + } + public void testCacheCleanupBasedOnStaleThreshold_StalenessGreaterThanThreshold() throws Exception { IndicesService indicesService = getInstanceFromNode(IndicesService.class); IndexShard indexShard = createIndex("test").getShard(0); @@ -637,35 +638,25 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessGreaterThanThreshold( DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - if (randomBoolean()) { - writer.flush(); - IOUtils.close(writer); - writer = new IndexWriter(dir, newIndexWriterConfig()); - } - writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); // Get 2 entries into the cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); cache.getOrCompute(entity, loader, reader, termBytes); - - entity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(1, cache.count()); IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); - cache.getOrCompute(entity, loader, secondReader, termBytes); - - secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); cache.getOrCompute(secondEntity, loader, secondReader, termBytes); assertEquals(2, cache.count()); + // no stale keys so far + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); // Close the reader, to be enqueued for cleanup - // 1 out of 2 keys ie 50% are now stale. reader.close(); + // 1 out of 2 keys ie 50% are now stale. + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); // cache count should not be affected assertEquals(2, cache.count()); @@ -673,6 +664,7 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessGreaterThanThreshold( cache.cacheCleanupManager.cleanCache(); // cleanup should have taken effect with 49% threshold assertEquals(1, cache.count()); + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); IOUtils.close(secondReader, writer, dir, cache); terminate(threadPool); @@ -699,47 +691,193 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessLesserThanThreshold() DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - if (randomBoolean()) { - writer.flush(); - IOUtils.close(writer); - writer = new IndexWriter(dir, newIndexWriterConfig()); - } - writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); // Get 2 entries into the cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); cache.getOrCompute(entity, loader, reader, termBytes); - - entity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(1, cache.count()); IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); - cache.getOrCompute(entity, loader, secondReader, termBytes); - - secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); cache.getOrCompute(secondEntity, loader, secondReader, termBytes); assertEquals(2, cache.count()); // Close the reader, to be enqueued for cleanup - // 1 out of 2 keys ie 50% are now stale. reader.close(); + // 1 out of 2 keys ie 50% are now stale. + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); // cache count should not be affected assertEquals(2, cache.count()); // clean cache with 51% staleness threshold cache.cacheCleanupManager.cleanCache(); // cleanup should have been ignored + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); assertEquals(2, cache.count()); IOUtils.close(secondReader, writer, dir, cache); terminate(threadPool); } + // test staleness count based on removal notifications + public void testStaleCount_OnRemovalNotifications() throws Exception { + IndicesService indicesService = getInstanceFromNode(IndicesService.class); + IndexShard indexShard = createIndex("test").getShard(0); + ThreadPool threadPool = getThreadPool(); + Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); + IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { + IndexService indexService = null; + try { + indexService = indicesService.indexServiceSafe(shardId.getIndex()); + } catch (IndexNotFoundException ex) { + return Optional.empty(); + } + return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); + }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); + Directory dir = newDirectory(); + IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + + writer.addDocument(newDoc(0, "foo")); + DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + + // Get 5 entries into the cache + int totalKeys = 5; + IndicesService.IndexShardCacheEntity entity = null; + TermQueryBuilder termQuery = null; + BytesReference termBytes = null; + for (int i = 1; i <= totalKeys; i++) { + termQuery = new TermQueryBuilder("id", "" + i); + termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + entity = new IndicesService.IndexShardCacheEntity(indexShard); + Loader loader = new Loader(reader, 0); + cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(i, cache.count()); + } + // no keys are stale yet + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + // closing the reader should make all keys stale + reader.close(); + assertEquals(totalKeys, cache.cacheCleanupManager.getStaleKeysCount().get()); + + String readerCacheKeyId = getReaderCacheKeyId(reader); + IndicesRequestCache.Key key = new IndicesRequestCache.Key( + ((IndexShard) entity.getCacheIdentity()).shardId(), + termBytes, + readerCacheKeyId + ); + + int staleCount = cache.cacheCleanupManager.getStaleKeysCount().get(); + // Notification for Replaced should not deduct the staleCount + cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.REPLACED)); + // stale keys count should stay the same + assertEquals(staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); + + // Notification for all but Replaced should deduct the staleCount + RemovalReason[] reasons = { RemovalReason.INVALIDATED, RemovalReason.EVICTED, RemovalReason.EXPLICIT, RemovalReason.CAPACITY }; + for (RemovalReason reason : reasons) { + cache.onRemoval(new RemovalNotification(key, termBytes, reason)); + assertEquals(--staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); + } + + IOUtils.close(writer, dir, cache); + terminate(threadPool); + } + + // test the cleanupKeyToCountMap are set appropriately when both readers are closed + public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { + IndicesService indicesService = getInstanceFromNode(IndicesService.class); + IndexShard indexShard = createIndex("test").getShard(0); + ShardId shardId = indexShard.shardId(); + ThreadPool threadPool = getThreadPool(); + Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); + IndicesRequestCache cache = new IndicesRequestCache(settings, (currentShardId -> { + IndexService indexService = null; + try { + indexService = indicesService.indexServiceSafe(shardId.getIndex()); + } catch (IndexNotFoundException ex) { + return Optional.empty(); + } + return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); + }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); + Directory dir = newDirectory(); + IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + writer.addDocument(newDoc(0, "foo")); + + DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); + BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + if (randomBoolean()) { + writer.flush(); + IOUtils.close(writer); + writer = new IndexWriter(dir, newIndexWriterConfig()); + } + writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); + + // Get 2 entries into the cache from 2 different readers + IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); + Loader loader = new Loader(reader, 0); + cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(1, cache.count()); + // test the mappings + String readerCacheKeyId = getReaderCacheKeyId(reader); + ConcurrentMap< + ShardId, + HashMap> cleanupKeyToCountMap = + cache.cacheCleanupManager.getCleanupKeyToCountMap(); + assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); + assertFalse(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + + IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); + loader = new Loader(secondReader, 0); + cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + // test the mapping + readerCacheKeyId = getReaderCacheKeyId(secondReader); + assertEquals(2, cache.count()); + assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); + assertFalse(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + + // Close the reader, to create stale entries + reader.close(); + // 1 out of 2 keys ie 50% are now stale. + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); + // cache count should not be affected + assertEquals(2, cache.count()); + + // test the mapping + readerCacheKeyId = getReaderCacheKeyId(reader); + assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); + assertTrue(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + + // second reader's mapping should not be affected + readerCacheKeyId = getReaderCacheKeyId(secondReader); + assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); + assertFalse(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + + // Close the second reader + secondReader.close(); + // both keys should now be stale + assertEquals(2, cache.cacheCleanupManager.getStaleKeysCount().get()); + // cache count should not be affected + assertEquals(2, cache.count()); + + // test the mapping + readerCacheKeyId = getReaderCacheKeyId(secondReader); + assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); + assertTrue(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + + IOUtils.close(writer, dir, cache); + terminate(threadPool); + } + + private String getReaderCacheKeyId(DirectoryReader reader) { + OpenSearchDirectoryReader.DelegatingCacheHelper delegatingCacheHelper = (OpenSearchDirectoryReader.DelegatingCacheHelper) reader + .getReaderCacheHelper(); + return delegatingCacheHelper.getDelegatingCacheKey().getId(); + } + public void testEviction() throws Exception { final ByteSizeValue size; { From 917df93dd0bbd2e34c51f8f6f2a31aa3f0f93ed1 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Wed, 3 Apr 2024 14:29:13 -0700 Subject: [PATCH 03/37] Update CHANGELOG.md Signed-off-by: Kiran Prakash --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f9febf7013a9..fffbe51a46b5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix bulk API ignores ingest pipeline for upsert ([#12883](https://github.com/opensearch-project/OpenSearch/pull/12883)) - Fix issue with feature flags where default value may not be honored ([#12849](https://github.com/opensearch-project/OpenSearch/pull/12849)) - Fix UOE While building Exists query for nested search_as_you_type field ([#12048](https://github.com/opensearch-project/OpenSearch/pull/12048)) +- Fix IndicesRequestCache Stale calculation to be accurate. ([#13070](https://github.com/opensearch-project/OpenSearch/pull/13070)] ### Security From ac1464f0b2a0849545be610181b131f7c4e2d868 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 4 Apr 2024 13:04:03 -0700 Subject: [PATCH 04/37] revert Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 74 ++++++------------- 1 file changed, 23 insertions(+), 51 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 566258b33ad02..930e6a291fcda 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -449,23 +449,8 @@ public int hashCode() { * If Staleness threshold is 0, we do not keep track of stale keys in the cache * */ class IndicesRequestCacheCleanupManager implements Closeable { - class StalenessProperty { - private int count; - private boolean isAccounted; - - // for testing - int getCount() { - return count; - } - - // for testing - boolean getIsAccounted() { - return isAccounted; - } - } - private final Set keysToClean; - private final ConcurrentMap> cleanupKeyToCountMap; + private final ConcurrentMap> cleanupKeyToCountMap; private final AtomicInteger staleKeysCount; private final double stalenessThreshold; private final IndicesRequestCacheCleaner cacheCleaner; @@ -514,19 +499,7 @@ private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { // If the key doesn't exist, it's added with a value of 1. // If the key exists, its value is incremented by 1. - cleanupKeyToCountMap.compute(shardId, (currentShardId, stalenessPropertyMap) -> { - if (stalenessPropertyMap == null) { - stalenessPropertyMap = new HashMap<>(); - } - stalenessPropertyMap.compute(cleanupKey.readerCacheKeyId, (readerCacheKey, stalenessProperty) -> { - if (stalenessProperty == null) { - stalenessProperty = new StalenessProperty(); - } - stalenessProperty.count += 1; - return stalenessProperty; - }); - return stalenessPropertyMap; - }); + cleanupKeyToCountMap.computeIfAbsent(shardId, k -> new HashMap<>()).merge(cleanupKey.readerCacheKeyId, 1, Integer::sum); } /** @@ -550,17 +523,15 @@ private void updateCleanupKeyToCountMapOnCacheEviction(CleanupKey cleanupKey) { } ShardId shardId = indexShard.shardId(); - cleanupKeyToCountMap.computeIfPresent(shardId, (shard, stalenessPropertyMap) -> { - stalenessPropertyMap.computeIfPresent(cleanupKey.readerCacheKeyId, (readerCacheKey, stalenessProperty) -> { - if (stalenessProperty.isAccounted) { - staleKeysCount.decrementAndGet(); - } - stalenessProperty.count -= 1; + cleanupKeyToCountMap.computeIfPresent(shardId, (shard, keyCountMap) -> { + keyCountMap.computeIfPresent(cleanupKey.readerCacheKeyId, (key, currentValue) -> { + // decrement the stale key count + staleKeysCount.decrementAndGet(); + int newValue = currentValue - 1; // Remove the key if the new value is zero by returning null; otherwise, update with the new value. - return stalenessProperty.count == 0 ? null : stalenessProperty; + return newValue == 0 ? null : newValue; }); - // If stalenessPropertyMap is empty after removal, return null to remove the entry for shardId - return stalenessPropertyMap.isEmpty() ? null : stalenessPropertyMap; + return keyCountMap; }); } @@ -586,26 +557,27 @@ private void incrementStaleKeysCount(CleanupKey cleanupKey) { ShardId shardId = indexShard.shardId(); // Using computeIfPresent to atomically operate on the countMap for a given shardId - cleanupKeyToCountMap.computeIfPresent(shardId, (currentShardId, stalenessPropertyMap) -> { + cleanupKeyToCountMap.computeIfPresent(shardId, (currentShardId, countMap) -> { if (cleanupKey.readerCacheKeyId == null) { // Aggregate and add to staleKeysCount atomically if readerCacheKeyId is null - int totalSum = stalenessPropertyMap.values().stream().mapToInt(stalenessProperty -> stalenessProperty.count).sum(); + int totalSum = countMap.values().stream().mapToInt(Integer::intValue).sum(); staleKeysCount.addAndGet(totalSum); - // Set isAccounted to true for all StalenessProperty in the map - stalenessPropertyMap.values().forEach(stalenessProperty -> stalenessProperty.isAccounted = true); // return the updated map - return stalenessPropertyMap; + return countMap; } else { // Update staleKeysCount based on specific readerCacheKeyId, then remove it from the countMap - stalenessPropertyMap.computeIfPresent(cleanupKey.readerCacheKeyId, (readerCacheKey, stalenessProperty) -> { - staleKeysCount.addAndGet(stalenessProperty.count); - stalenessProperty.isAccounted = true; - // return the updated stalenessProperty - return stalenessProperty; + countMap.computeIfPresent(cleanupKey.readerCacheKeyId, (readerCacheKey, count) -> { + staleKeysCount.addAndGet(count); + // Return null to remove the key after updating staleKeysCount + return null; }); + // Check if countMap is empty after removal to decide if we need to remove the shardId entry + if (countMap.isEmpty()) { + return null; // Returning null removes the entry for shardId + } } - // Return the modified stalenessPropertyMap - return stalenessPropertyMap; + // Return the modified countMap + return countMap; }); } @@ -720,7 +692,7 @@ public void close() { this.cacheCleaner.close(); } - public ConcurrentMap> getCleanupKeyToCountMap() { + public ConcurrentMap> getCleanupKeyToCountMap() { return cleanupKeyToCountMap; } From f2d72f29c23e374856aba107eca5c5083946fed9 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 4 Apr 2024 13:05:37 -0700 Subject: [PATCH 05/37] revert Signed-off-by: Kiran Prakash --- .../java/org/opensearch/indices/IndicesRequestCache.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 930e6a291fcda..a1be9f492c8b1 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -562,8 +562,8 @@ private void incrementStaleKeysCount(CleanupKey cleanupKey) { // Aggregate and add to staleKeysCount atomically if readerCacheKeyId is null int totalSum = countMap.values().stream().mapToInt(Integer::intValue).sum(); staleKeysCount.addAndGet(totalSum); - // return the updated map - return countMap; + // Return null to automatically remove the mapping for shardId + return null; } else { // Update staleKeysCount based on specific readerCacheKeyId, then remove it from the countMap countMap.computeIfPresent(cleanupKey.readerCacheKeyId, (readerCacheKey, count) -> { @@ -573,7 +573,8 @@ private void incrementStaleKeysCount(CleanupKey cleanupKey) { }); // Check if countMap is empty after removal to decide if we need to remove the shardId entry if (countMap.isEmpty()) { - return null; // Returning null removes the entry for shardId + // Returning null removes the entry for shardId + return null; } } // Return the modified countMap From 148ff743c176f804dd49750f64cb688b4c32f8df Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 4 Apr 2024 14:04:31 -0700 Subject: [PATCH 06/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index a1be9f492c8b1..84461bf383f60 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -523,15 +523,30 @@ private void updateCleanupKeyToCountMapOnCacheEviction(CleanupKey cleanupKey) { } ShardId shardId = indexShard.shardId(); - cleanupKeyToCountMap.computeIfPresent(shardId, (shard, keyCountMap) -> { - keyCountMap.computeIfPresent(cleanupKey.readerCacheKeyId, (key, currentValue) -> { - // decrement the stale key count - staleKeysCount.decrementAndGet(); - int newValue = currentValue - 1; - // Remove the key if the new value is zero by returning null; otherwise, update with the new value. + cleanupKeyToCountMap.computeIfPresent(shardId, (id, stringIntegerConcurrentMap) -> { + stringIntegerConcurrentMap.compute(cleanupKey.readerCacheKeyId, (readerCacheKeyId, count) -> { + // Check if the key is currently present + if (count != null) { + // The key does not exist, so this is already accounted in the staleKeysCount + // and hence needs to be decremented + staleKeysCount.decrementAndGet(); + } + + // Regardless of whether the key was initially present, we perform the decrement operation + // Calculate the new value assuming a null count as zero to handle non-existent keys + int newValue = (count == null ? 0 : count) - 1; + + // If the new value is 0, remove the entry by returning null return newValue == 0 ? null : newValue; }); - return keyCountMap; + + // return null and remove the shardId entry if the shardId has no other entries + if (stringIntegerConcurrentMap.isEmpty()) { + return null; + } + + // Return the modified map to ensure it gets updated + return stringIntegerConcurrentMap; }); } @@ -577,7 +592,7 @@ private void incrementStaleKeysCount(CleanupKey cleanupKey) { return null; } } - // Return the modified countMap + // Return the modified countMap to retain updates return countMap; }); } From d8451c27fa5f1c36ed5dc58bc1758533aac2bf97 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 4 Apr 2024 14:04:35 -0700 Subject: [PATCH 07/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCacheTests.java | 26 +++++-------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index f99249e4dae00..7885926018e7f 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -809,12 +809,6 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - if (randomBoolean()) { - writer.flush(); - IOUtils.close(writer); - writer = new IndexWriter(dir, newIndexWriterConfig()); - } - writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); // Get 2 entries into the cache from 2 different readers IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); @@ -823,12 +817,8 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { assertEquals(1, cache.count()); // test the mappings String readerCacheKeyId = getReaderCacheKeyId(reader); - ConcurrentMap< - ShardId, - HashMap> cleanupKeyToCountMap = - cache.cacheCleanupManager.getCleanupKeyToCountMap(); - assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); - assertFalse(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + ConcurrentMap> cleanupKeyToCountMap = cache.cacheCleanupManager.getCleanupKeyToCountMap(); + assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); @@ -836,8 +826,7 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { // test the mapping readerCacheKeyId = getReaderCacheKeyId(secondReader); assertEquals(2, cache.count()); - assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); - assertFalse(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); // Close the reader, to create stale entries reader.close(); @@ -848,13 +837,11 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { // test the mapping readerCacheKeyId = getReaderCacheKeyId(reader); - assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); - assertTrue(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); // second reader's mapping should not be affected readerCacheKeyId = getReaderCacheKeyId(secondReader); - assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); - assertFalse(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); // Close the second reader secondReader.close(); @@ -865,8 +852,7 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { // test the mapping readerCacheKeyId = getReaderCacheKeyId(secondReader); - assertEquals(1, cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getCount()); - assertTrue(cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId).getIsAccounted()); + assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); IOUtils.close(writer, dir, cache); terminate(threadPool); From ad889361e8afe852a3887f2a16f52ee82066ff99 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 4 Apr 2024 14:09:25 -0700 Subject: [PATCH 08/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../main/java/org/opensearch/indices/IndicesRequestCache.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 84461bf383f60..6cfe7eda16b81 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -708,6 +708,7 @@ public void close() { this.cacheCleaner.close(); } + // for testing public ConcurrentMap> getCleanupKeyToCountMap() { return cleanupKeyToCountMap; } From 87e5c46042575185e9d769b2122b9a60cf040436 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 4 Apr 2024 15:40:41 -0700 Subject: [PATCH 09/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../java/org/opensearch/indices/IndicesRequestCacheTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 7885926018e7f..fe7ee052e80e1 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -508,7 +508,7 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( terminate(threadPool); } - // when a cache entry that is NOT Stale is evicted for any reason, we should NOT deduct it from staleness count + // when a cache entry that is NOT Stale is evicted for any reason, staleness count should NOT be deducted public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStaleCount() throws Exception { IndicesService indicesService = getInstanceFromNode(IndicesService.class); IndexShard indexShard = createIndex("test").getShard(0); From 6b36ee6ee084ad4b56b77a2e7a71769c75f448f4 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Mon, 8 Apr 2024 10:33:05 -0700 Subject: [PATCH 10/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 67 ++++++++++--------- 1 file changed, 36 insertions(+), 31 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 6cfe7eda16b81..4ce94714d3bed 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -208,17 +208,17 @@ public void onRemoval(RemovalNotification notification) { // shards as part of request cache. Key key = notification.getKey(); cacheEntityLookup.apply(key.shardId).ifPresent(entity -> entity.onRemoval(notification)); - if (notificationAffectsStaleness(notification)) { - CleanupKey cleanupKey = new CleanupKey(cacheEntityLookup.apply(key.shardId).orElse(null), key.readerCacheKeyId); - cacheCleanupManager.updateCleanupKeyToCountMapOnCacheEviction(cleanupKey); - } + CleanupKey cleanupKey = new CleanupKey(cacheEntityLookup.apply(key.shardId).orElse(null), key.readerCacheKeyId); + cacheCleanupManager.updateCleanupKeyCountOnKeyRemoval(cleanupKey, notification); } /** * This method checks if the removal reason of the notification is not REPLACED. - * The reason of the notification is REPLACED when a cache entry's value is updated. - * If the removal reason is anything other than REPLACED, it will return true. - * If the removal reason is REPLACED, it will return false. + * The reason of the notification is REPLACED when a cache entry's value is updated, since replacing an entry + * does not affect the staleness count, we skip such notifications. + * + *

If the removal reason is anything other than REPLACED, this will return true. + * If the removal reason is REPLACED, this will return false. */ private boolean notificationAffectsStaleness(RemovalNotification notification) { return notification.getRemovalReason() != RemovalReason.REPLACED; @@ -510,10 +510,14 @@ private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { * It also decrements the staleKeysCount only if the entry was accounted. * If the count of the CleanupKey becomes zero, it removes the CleanupKey from the map. * - * @param cleanupKey the CleanupKey that has been evicted from the cache + *

We update the cleanupKeyToCountMap on every key removed from cache + * except for the keys removed with reason Replaced + * + * @param cleanupKey the CleanupKey that has been evicted from the cache + * @param notification RemovalNotification of the cache entry evicted */ - private void updateCleanupKeyToCountMapOnCacheEviction(CleanupKey cleanupKey) { - if (stalenessThreshold == 0.0 || cleanupKey.entity == null) { + private void updateCleanupKeyCountOnKeyRemoval(CleanupKey cleanupKey, RemovalNotification notification) { + if (!notificationAffectsStaleness(notification) || stalenessThreshold == 0.0 || cleanupKey.entity == null) { return; } IndexShard indexShard = (IndexShard) cleanupKey.entity.getCacheIdentity(); @@ -523,30 +527,31 @@ private void updateCleanupKeyToCountMapOnCacheEviction(CleanupKey cleanupKey) { } ShardId shardId = indexShard.shardId(); - cleanupKeyToCountMap.computeIfPresent(shardId, (id, stringIntegerConcurrentMap) -> { - stringIntegerConcurrentMap.compute(cleanupKey.readerCacheKeyId, (readerCacheKeyId, count) -> { - // Check if the key is currently present - if (count != null) { - // The key does not exist, so this is already accounted in the staleKeysCount - // and hence needs to be decremented + cleanupKeyToCountMap.compute(shardId, (key, readerCacheKeyMap) -> { + if (readerCacheKeyMap == null || !readerCacheKeyMap.containsKey(cleanupKey.readerCacheKeyId)) { + // If ShardId is not present or readerCacheKeyId is not present, decrement staleKeysCount + staleKeysCount.decrementAndGet(); + return null; // Returning null removes the entry for the shardId, if it exists + } else { + // Proceed to adjust the count for the readerCacheKeyId + Integer count = readerCacheKeyMap.get(cleanupKey.readerCacheKeyId); + if (count == null) { staleKeysCount.decrementAndGet(); + } else { + // Reduce the count by 1 + int newCount = count - 1; + if (newCount <= 0) { + // Remove the readerCacheKeyId entry if new count is zero or less + readerCacheKeyMap.remove(cleanupKey.readerCacheKeyId); + } else { + // Update the map with the new count + readerCacheKeyMap.put(cleanupKey.readerCacheKeyId, newCount); + } } - // Regardless of whether the key was initially present, we perform the decrement operation - // Calculate the new value assuming a null count as zero to handle non-existent keys - int newValue = (count == null ? 0 : count) - 1; - - // If the new value is 0, remove the entry by returning null - return newValue == 0 ? null : newValue; - }); - - // return null and remove the shardId entry if the shardId has no other entries - if (stringIntegerConcurrentMap.isEmpty()) { - return null; + // If after modification, the readerCacheKeyMap is empty, we return null to remove the ShardId entry + return readerCacheKeyMap.isEmpty() ? null : readerCacheKeyMap; } - - // Return the modified map to ensure it gets updated - return stringIntegerConcurrentMap; }); } @@ -709,7 +714,7 @@ public void close() { } // for testing - public ConcurrentMap> getCleanupKeyToCountMap() { + ConcurrentMap> getCleanupKeyToCountMap() { return cleanupKeyToCountMap; } From bebfc3ade7e8e0377435a046b4ce3521af0a7a67 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Mon, 8 Apr 2024 10:33:09 -0700 Subject: [PATCH 11/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCacheTests.java | 666 +++++------------- 1 file changed, 183 insertions(+), 483 deletions(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index fe7ee052e80e1..7796644ab7689 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -38,7 +38,6 @@ import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.IndexWriter; -import org.apache.lucene.index.IndexWriterConfig; import org.apache.lucene.index.Term; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.TermQuery; @@ -49,7 +48,6 @@ import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.module.CacheModule; -import org.opensearch.common.cache.service.CacheService; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.lucene.index.OpenSearchDirectoryReader; import org.opensearch.common.settings.Settings; @@ -72,6 +70,8 @@ import org.opensearch.node.Node; import org.opensearch.test.OpenSearchSingleNodeTestCase; import org.opensearch.threadpool.ThreadPool; +import org.junit.After; +import org.junit.Before; import java.io.IOException; import java.util.ArrayList; @@ -87,31 +87,39 @@ import static org.mockito.Mockito.when; public class IndicesRequestCacheTests extends OpenSearchSingleNodeTestCase { + private ThreadPool threadPool; + private IndexWriter writer; + private Directory dir; + private IndicesRequestCache cache; + private IndexShard indexShard; + private ThreadPool getThreadPool() { return new ThreadPool(Settings.builder().put(Node.NODE_NAME_SETTING.getKey(), "default tracer tests").build()); } - public void testBasicOperationsCache() throws Exception { - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); - IndicesRequestCache cache = new IndicesRequestCache( - Settings.EMPTY, - (shardId -> Optional.of(new IndicesService.IndexShardCacheEntity(indexShard))), - new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), - threadPool - ); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + @Before + public void setup() throws IOException { + dir = newDirectory(); + writer = new IndexWriter(dir, newIndexWriterConfig()); + indexShard = createIndex("test").getShard(0); + } + @After + public void cleanup() throws IOException { + IOUtils.close(writer, dir, cache); + terminate(threadPool); + } + + public void testBasicOperationsCache() throws Exception { + threadPool = getThreadPool(); + cache = getIndicesRequestCache(Settings.EMPTY, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + DirectoryReader reader = getReader(writer, indexShard.shardId()); // initial cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); + BytesReference value = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value.streamInput().readString()); ShardRequestCache requestCacheStats = indexShard.requestCache(); assertEquals(0, requestCacheStats.stats().getHitCount()); @@ -123,7 +131,7 @@ public void testBasicOperationsCache() throws Exception { // cache hit entity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(reader, 0); - value = cache.getOrCompute(entity, loader, reader, termBytes); + value = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value.streamInput().readString()); requestCacheStats = indexShard.requestCache(); assertEquals(1, requestCacheStats.stats().getHitCount()); @@ -149,33 +157,21 @@ public void testBasicOperationsCache() throws Exception { assertEquals(0, cache.count()); assertEquals(0, requestCacheStats.stats().getMemorySize().bytesAsInt()); - IOUtils.close(reader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(reader); assertEquals(0, cache.numRegisteredCloseListeners()); } public void testBasicOperationsCacheWithFeatureFlag() throws Exception { - IndexShard indexShard = createIndex("test").getShard(0); - CacheService cacheService = new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(); - ThreadPool threadPool = getThreadPool(); - IndicesRequestCache cache = new IndicesRequestCache( - Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.PLUGGABLE_CACHE, "true").build(), - (shardId -> Optional.of(new IndicesService.IndexShardCacheEntity(indexShard))), - cacheService, - threadPool - ); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + threadPool = getThreadPool(); + Settings settings = Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.PLUGGABLE_CACHE, "true").build(); + cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + DirectoryReader reader = getReader(writer, indexShard.shardId()); // initial cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); + BytesReference value = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value.streamInput().readString()); ShardRequestCache requestCacheStats = indexShard.requestCache(); assertEquals(0, requestCacheStats.stats().getHitCount()); @@ -187,7 +183,7 @@ public void testBasicOperationsCacheWithFeatureFlag() throws Exception { // cache hit entity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(reader, 0); - value = cache.getOrCompute(entity, loader, reader, termBytes); + value = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value.streamInput().readString()); requestCacheStats = indexShard.requestCache(); assertEquals(1, requestCacheStats.stats().getHitCount()); @@ -213,43 +209,28 @@ public void testBasicOperationsCacheWithFeatureFlag() throws Exception { assertEquals(0, cache.count()); assertEquals(0, requestCacheStats.stats().getMemorySize().bytesAsInt()); - IOUtils.close(reader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(reader); assertEquals(0, cache.numRegisteredCloseListeners()); } public void testCacheDifferentReaders() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); - IndicesRequestCache cache = new IndicesRequestCache(Settings.EMPTY, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + threadPool = getThreadPool(); + cache = getIndicesRequestCache(Settings.EMPTY, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + if (randomBoolean()) { writer.flush(); IOUtils.close(writer); writer = new IndexWriter(dir, newIndexWriterConfig()); } writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); // initial cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); + BytesReference value = cache.getOrCompute(entity, loader, reader, getTermBytes()); ShardRequestCache requestCacheStats = entity.stats(); assertEquals("foo", value.streamInput().readString()); assertEquals(0, requestCacheStats.stats().getHitCount()); @@ -264,7 +245,7 @@ public void testCacheDifferentReaders() throws Exception { // cache the second IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); - value = cache.getOrCompute(entity, loader, secondReader, termBytes); + value = cache.getOrCompute(entity, loader, secondReader, getTermBytes()); requestCacheStats = entity.stats(); assertEquals("bar", value.streamInput().readString()); assertEquals(0, requestCacheStats.stats().getHitCount()); @@ -277,7 +258,7 @@ public void testCacheDifferentReaders() throws Exception { secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(secondReader, 0); - value = cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + value = cache.getOrCompute(secondEntity, loader, secondReader, getTermBytes()); requestCacheStats = entity.stats(); assertEquals("bar", value.streamInput().readString()); assertEquals(1, requestCacheStats.stats().getHitCount()); @@ -288,7 +269,7 @@ public void testCacheDifferentReaders() throws Exception { entity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(reader, 0); - value = cache.getOrCompute(entity, loader, reader, termBytes); + value = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value.streamInput().readString()); requestCacheStats = entity.stats(); assertEquals(2, requestCacheStats.stats().getHitCount()); @@ -321,8 +302,7 @@ public void testCacheDifferentReaders() throws Exception { assertEquals(0, cache.count()); assertEquals(0, requestCacheStats.stats().getMemorySize().bytesAsInt()); - IOUtils.close(secondReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(secondReader); assertEquals(0, cache.numRegisteredCloseListeners()); } @@ -350,38 +330,18 @@ public void testCacheCleanupThresholdSettingValidator_Invalid_Percentage() { } public void testCacheCleanupBasedOnZeroThreshold() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0%").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); // Get 2 entries into the cache - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals(1, cache.count()); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); - secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals(2, cache.count()); // Close the reader, to be enqueued for cleanup @@ -393,43 +353,22 @@ public void testCacheCleanupBasedOnZeroThreshold() throws Exception { cache.cacheCleanupManager.cleanCache(); // cleanup should remove the stale-key assertEquals(1, cache.count()); - - IOUtils.close(secondReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(secondReader); } public void testCacheCleanupBasedOnStaleThreshold_StalenessEqualToThreshold() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.5").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); // Get 2 entries into the cache - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals(1, cache.count()); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); - cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals(2, cache.count()); // Close the reader, to be enqueued for cleanup @@ -445,44 +384,23 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessEqualToThreshold() th assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); assertEquals(1, cache.count()); - IOUtils.close(secondReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(secondReader); } // when a cache entry that is Stale is evicted for any reason, we have to deduct the count from our staleness count public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - ShardId shardId = new ShardId("foo", "bar", 1); - + cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); // Get 2 entries into the cache from 2 different readers - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals(1, cache.count()); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); - cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals(2, cache.count()); // assert no stale keys are accounted so far @@ -494,53 +412,29 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( // cache count should not be affected assertEquals(2, cache.count()); - IndicesRequestCache.Key key = new IndicesRequestCache.Key( - ((IndexShard) secondEntity.getCacheIdentity()).shardId(), - termBytes, - getReaderCacheKeyId(reader) - ); + IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(reader)); - cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.EVICTED)); + cache.onRemoval(new RemovalNotification(key, getTermBytes(), RemovalReason.EVICTED)); // eviction of previous stale key from the cache should decrement staleKeysCount in iRC assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); - IOUtils.close(secondReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(secondReader); } // when a cache entry that is NOT Stale is evicted for any reason, staleness count should NOT be deducted public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStaleCount() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); // Get 2 entries into the cache - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals(1, cache.count()); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); - cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals(2, cache.count()); // Close the reader, to be enqueued for cleanup @@ -552,103 +446,59 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStal assertEquals(2, cache.count()); // evict entry from second reader (this reader is not closed) - String readerCacheKeyId = getReaderCacheKeyId(secondReader); - IndicesRequestCache.Key key = new IndicesRequestCache.Key( - ((IndexShard) secondEntity.getCacheIdentity()).shardId(), - termBytes, - readerCacheKeyId - ); + IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(secondReader)); - cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.EVICTED)); + cache.onRemoval(new RemovalNotification(key, getTermBytes(), RemovalReason.EVICTED)); staleKeysCount = cache.cacheCleanupManager.getStaleKeysCount(); // eviction of NON-stale key from the cache should NOT decrement staleKeysCount in iRC assertEquals(1, staleKeysCount.get()); - IOUtils.close(secondReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(secondReader); } // when a cache entry that is NOT Stale is evicted WITHOUT its reader closing, we should NOT deduct it from staleness count - public void testStaleCount_WithoutReaderClosing_OnRemovalNotificationOfStaleKey_DecrementsStaleCount() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); + public void testStaleCount_WithoutReaderClosing_DecrementsStaleCount() throws Exception { + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); // Get 2 entries into the cache from 2 different readers - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); + assertEquals(1, cache.count()); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); - cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals(2, cache.count()); + // no keys are stale assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); // create notification for removal of non-stale entry - IndicesRequestCache.Key key = new IndicesRequestCache.Key( - ((IndexShard) secondEntity.getCacheIdentity()).shardId(), - termBytes, - getReaderCacheKeyId(reader) - ); - cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.EVICTED)); + IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(reader)); + cache.onRemoval(new RemovalNotification(key, getTermBytes(), RemovalReason.EVICTED)); // stale keys count should stay zero assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); - IOUtils.close(reader, secondReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(reader, secondReader); } - public void testCacheCleanupBasedOnStaleThreshold_StalenessGreaterThanThreshold() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); + // test staleness count based on removal notifications + public void testStaleCount_OnRemovalNotifications() throws Exception { + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.49").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); // Get 2 entries into the cache - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals(1, cache.count()); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); - cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), secondReader, getTermBytes()); assertEquals(2, cache.count()); // no stale keys so far @@ -666,42 +516,23 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessGreaterThanThreshold( assertEquals(1, cache.count()); assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); - IOUtils.close(secondReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(secondReader); } public void testCacheCleanupBasedOnStaleThreshold_StalenessLesserThanThreshold() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "51%").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); // Get 2 entries into the cache - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals(1, cache.count()); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); - cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals(2, cache.count()); // Close the reader, to be enqueued for cleanup @@ -717,116 +548,31 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessLesserThanThreshold() assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); assertEquals(2, cache.count()); - IOUtils.close(secondReader, writer, dir, cache); - terminate(threadPool); - } - - // test staleness count based on removal notifications - public void testStaleCount_OnRemovalNotifications() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); - Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - - writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - - // Get 5 entries into the cache - int totalKeys = 5; - IndicesService.IndexShardCacheEntity entity = null; - TermQueryBuilder termQuery = null; - BytesReference termBytes = null; - for (int i = 1; i <= totalKeys; i++) { - termQuery = new TermQueryBuilder("id", "" + i); - termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); - assertEquals(i, cache.count()); - } - // no keys are stale yet - assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); - // closing the reader should make all keys stale - reader.close(); - assertEquals(totalKeys, cache.cacheCleanupManager.getStaleKeysCount().get()); - - String readerCacheKeyId = getReaderCacheKeyId(reader); - IndicesRequestCache.Key key = new IndicesRequestCache.Key( - ((IndexShard) entity.getCacheIdentity()).shardId(), - termBytes, - readerCacheKeyId - ); - - int staleCount = cache.cacheCleanupManager.getStaleKeysCount().get(); - // Notification for Replaced should not deduct the staleCount - cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.REPLACED)); - // stale keys count should stay the same - assertEquals(staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); - - // Notification for all but Replaced should deduct the staleCount - RemovalReason[] reasons = { RemovalReason.INVALIDATED, RemovalReason.EVICTED, RemovalReason.EXPLICIT, RemovalReason.CAPACITY }; - for (RemovalReason reason : reasons) { - cache.onRemoval(new RemovalNotification(key, termBytes, reason)); - assertEquals(--staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); - } - - IOUtils.close(writer, dir, cache); - terminate(threadPool); + IOUtils.close(secondReader); } // test the cleanupKeyToCountMap are set appropriately when both readers are closed public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ShardId shardId = indexShard.shardId(); - ThreadPool threadPool = getThreadPool(); + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (currentShardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), settings).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - writer.addDocument(newDoc(0, "foo")); + cache = getIndicesRequestCache(settings, threadPool); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + writer.addDocument(newDoc(0, "foo")); + ShardId shardId = indexShard.shardId(); + DirectoryReader reader = getReader(writer, shardId); + DirectoryReader secondReader = getReader(writer, shardId); // Get 2 entries into the cache from 2 different readers - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - cache.getOrCompute(entity, loader, reader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals(1, cache.count()); // test the mappings - String readerCacheKeyId = getReaderCacheKeyId(reader); ConcurrentMap> cleanupKeyToCountMap = cache.cacheCleanupManager.getCleanupKeyToCountMap(); - assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); + assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(reader))); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - loader = new Loader(secondReader, 0); - cache.getOrCompute(secondEntity, loader, secondReader, termBytes); + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); // test the mapping - readerCacheKeyId = getReaderCacheKeyId(secondReader); assertEquals(2, cache.count()); - assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); + assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(secondReader))); // Close the reader, to create stale entries reader.close(); @@ -836,12 +582,10 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { assertEquals(2, cache.count()); // test the mapping - readerCacheKeyId = getReaderCacheKeyId(reader); - assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); + assertFalse(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(reader))); // second reader's mapping should not be affected - readerCacheKeyId = getReaderCacheKeyId(secondReader); - assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); + assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(secondReader))); // Close the second reader secondReader.close(); @@ -851,11 +595,39 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { assertEquals(2, cache.count()); // test the mapping - readerCacheKeyId = getReaderCacheKeyId(secondReader); - assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(readerCacheKeyId)); + // since all the readers of this shard is closed, + // the cleanupKeyToCountMap should have no entries + assertEquals(0, cleanupKeyToCountMap.size()); + } - IOUtils.close(writer, dir, cache); - terminate(threadPool); + private DirectoryReader getReader(IndexWriter writer, ShardId shardId) throws IOException { + return OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); + } + + private IndicesRequestCache getIndicesRequestCache(Settings settings, ThreadPool threadPool) { + IndicesService indicesService = getInstanceFromNode(IndicesService.class); + return new IndicesRequestCache(settings, (shardId -> { + IndexService indexService = null; + try { + indexService = indicesService.indexServiceSafe(shardId.getIndex()); + } catch (IndexNotFoundException ex) { + return Optional.empty(); + } + return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); + }), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool); + } + + private Loader getLoader(DirectoryReader reader) { + return new Loader(reader, 0); + } + + private IndicesService.IndexShardCacheEntity getEntity(IndexShard indexShard) { + return new IndicesService.IndexShardCacheEntity(indexShard); + } + + private BytesReference getTermBytes() throws IOException { + TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); + return XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); } private String getReaderCacheKeyId(DirectoryReader reader) { @@ -867,118 +639,73 @@ private String getReaderCacheKeyId(DirectoryReader reader) { public void testEviction() throws Exception { final ByteSizeValue size; { - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); - IndicesRequestCache cache = new IndicesRequestCache( - Settings.EMPTY, - (shardId -> Optional.of(new IndicesService.IndexShardCacheEntity(indexShard))), - new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), - threadPool - ); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + threadPool = getThreadPool(); + cache = getIndicesRequestCache(Settings.EMPTY, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - + DirectoryReader reader = getReader(writer, indexShard.shardId()); writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader secondLoader = new Loader(secondReader, 0); - BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes); + BytesReference value1 = cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals("foo", value1.streamInput().readString()); - BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes); + BytesReference value2 = cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals("bar", value2.streamInput().readString()); size = indexShard.requestCache().stats().getMemorySize(); IOUtils.close(reader, secondReader, writer, dir, cache); - terminate(threadPool); } - IndexShard indexShard = createIndex("test1").getShard(0); - ThreadPool threadPool = getThreadPool(); + indexShard = createIndex("test1").getShard(0); IndicesRequestCache cache = new IndicesRequestCache( Settings.builder().put(IndicesRequestCache.INDICES_CACHE_QUERY_SIZE.getKey(), size.getBytes() + 1 + "b").build(), (shardId -> Optional.of(new IndicesService.IndexShardCacheEntity(indexShard))), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool ); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + dir = newDirectory(); + writer = new IndexWriter(dir, newIndexWriterConfig()); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader loader = new Loader(reader, 0); - + DirectoryReader reader = getReader(writer, indexShard.shardId()); writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader secondLoader = new Loader(secondReader, 0); - + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); writer.updateDocument(new Term("id", "0"), newDoc(0, "baz")); DirectoryReader thirdReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - IndicesService.IndexShardCacheEntity thirddEntity = new IndicesService.IndexShardCacheEntity(indexShard); - Loader thirdLoader = new Loader(thirdReader, 0); - BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes); + BytesReference value1 = cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals("foo", value1.streamInput().readString()); - BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes); + BytesReference value2 = cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals("bar", value2.streamInput().readString()); logger.info("Memory size: {}", indexShard.requestCache().stats().getMemorySize()); - BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes); + BytesReference value3 = cache.getOrCompute(getEntity(indexShard), getLoader(thirdReader), thirdReader, getTermBytes()); assertEquals("baz", value3.streamInput().readString()); assertEquals(2, cache.count()); assertEquals(1, indexShard.requestCache().stats().getEvictions()); - IOUtils.close(reader, secondReader, thirdReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(reader, secondReader, thirdReader); } public void testClearAllEntityIdentity() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); - IndicesRequestCache cache = new IndicesRequestCache(Settings.EMPTY, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool); - - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + threadPool = getThreadPool(); + cache = getIndicesRequestCache(Settings.EMPTY, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + DirectoryReader reader = getReader(writer, indexShard.shardId()); IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); - DirectoryReader secondReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); IndicesService.IndexShardCacheEntity secondEntity = new IndicesService.IndexShardCacheEntity(indexShard); Loader secondLoader = new Loader(secondReader, 0); writer.updateDocument(new Term("id", "0"), newDoc(0, "baz")); - DirectoryReader thirdReader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); + DirectoryReader thirdReader = getReader(writer, indexShard.shardId()); + ; IndicesService.IndexShardCacheEntity thirddEntity = new IndicesService.IndexShardCacheEntity(createIndex("test1").getShard(0)); Loader thirdLoader = new Loader(thirdReader, 0); - BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes); + BytesReference value1 = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value1.streamInput().readString()); - BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes); + BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, getTermBytes()); assertEquals("bar", value2.streamInput().readString()); logger.info("Memory size: {}", indexShard.requestCache().stats().getMemorySize()); - BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes); + BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, getTermBytes()); assertEquals("baz", value3.streamInput().readString()); assertEquals(3, cache.count()); RequestCacheStats requestCacheStats = entity.stats().stats(); @@ -989,14 +716,13 @@ public void testClearAllEntityIdentity() throws Exception { cache.cacheCleanupManager.cleanCache(); assertEquals(1, cache.count()); // third has not been validated since it's a different identity - value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes); + value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, getTermBytes()); requestCacheStats = entity.stats().stats(); requestCacheStats.add(thirddEntity.stats().stats()); assertEquals(hitCount + 1, requestCacheStats.getHitCount()); assertEquals("baz", value3.streamInput().readString()); - IOUtils.close(reader, secondReader, thirdReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(reader, secondReader, thirdReader); } public Iterable newDoc(int id, String value) { @@ -1032,34 +758,18 @@ public BytesReference get() { throw new RuntimeException(e); } } - } public void testInvalidate() throws Exception { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - IndexShard indexShard = createIndex("test").getShard(0); - ThreadPool threadPool = getThreadPool(); - IndicesRequestCache cache = new IndicesRequestCache(Settings.EMPTY, (shardId -> { - IndexService indexService = null; - try { - indexService = indicesService.indexServiceSafe(shardId.getIndex()); - } catch (IndexNotFoundException ex) { - return Optional.empty(); - } - return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); - + threadPool = getThreadPool(); + IndicesRequestCache cache = getIndicesRequestCache(Settings.EMPTY, threadPool); writer.addDocument(newDoc(0, "foo")); - DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + DirectoryReader reader = getReader(writer, indexShard.shardId()); // initial cache IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); Loader loader = new Loader(reader, 0); - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); + BytesReference value = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value.streamInput().readString()); ShardRequestCache requestCacheStats = entity.stats(); assertEquals(0, requestCacheStats.stats().getHitCount()); @@ -1071,7 +781,7 @@ public void testInvalidate() throws Exception { // cache hit entity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(reader, 0); - value = cache.getOrCompute(entity, loader, reader, termBytes); + value = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value.streamInput().readString()); requestCacheStats = entity.stats(); assertEquals(1, requestCacheStats.stats().getHitCount()); @@ -1085,8 +795,8 @@ public void testInvalidate() throws Exception { // load again after invalidate entity = new IndicesService.IndexShardCacheEntity(indexShard); loader = new Loader(reader, 0); - cache.invalidate(entity, reader, termBytes); - value = cache.getOrCompute(entity, loader, reader, termBytes); + cache.invalidate(entity, reader, getTermBytes()); + value = cache.getOrCompute(entity, loader, reader, getTermBytes()); assertEquals("foo", value.streamInput().readString()); requestCacheStats = entity.stats(); assertEquals(1, requestCacheStats.stats().getHitCount()); @@ -1111,16 +821,11 @@ public void testInvalidate() throws Exception { assertEquals(0, cache.count()); assertEquals(0, requestCacheStats.stats().getMemorySize().bytesAsInt()); - IOUtils.close(reader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(reader); assertEquals(0, cache.numRegisteredCloseListeners()); } public void testEqualsKey() throws IOException { - IndicesService indicesService = getInstanceFromNode(IndicesService.class); - Directory dir = newDirectory(); - IndexWriterConfig config = newIndexWriterConfig(); - IndexWriter writer = new IndexWriter(dir, config); ShardId shardId = new ShardId("foo", "bar", 1); ShardId shardId1 = new ShardId("foo1", "bar1", 2); IndexReader reader1 = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); @@ -1147,13 +852,9 @@ public void testEqualsKey() throws IOException { } public void testSerializationDeserializationOfCacheKey() throws Exception { - TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); - IndexService indexService = createIndex("test"); - IndexShard indexShard = indexService.getShard(0); IndicesService.IndexShardCacheEntity shardCacheEntity = new IndicesService.IndexShardCacheEntity(indexShard); String readerCacheKeyId = UUID.randomUUID().toString(); - IndicesRequestCache.Key key1 = new IndicesRequestCache.Key(indexShard.shardId(), termBytes, readerCacheKeyId); + IndicesRequestCache.Key key1 = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), readerCacheKeyId); BytesReference bytesReference = null; try (BytesStreamOutput out = new BytesStreamOutput()) { key1.writeTo(out); @@ -1165,8 +866,7 @@ public void testSerializationDeserializationOfCacheKey() throws Exception { assertEquals(readerCacheKeyId, key2.readerCacheKeyId); assertEquals(((IndexShard) shardCacheEntity.getCacheIdentity()).shardId(), key2.shardId); - assertEquals(termBytes, key2.value); - + assertEquals(getTermBytes(), key2.value); } private class TestBytesReference extends AbstractBytesReference { From f42ebb87e42f2195b0f325ab86b61ca7165c2b84 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Mon, 8 Apr 2024 11:06:47 -0700 Subject: [PATCH 12/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCacheTests.java | 94 ++++++++++++++----- 1 file changed, 73 insertions(+), 21 deletions(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 7796644ab7689..8c817d37ff470 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -329,6 +329,7 @@ public void testCacheCleanupThresholdSettingValidator_Invalid_Percentage() { assertThrows(IllegalArgumentException.class, () -> { IndicesRequestCache.validateStalenessSetting("500%"); }); } + // when staleness threshold is zero, stale keys should be cleaned up every time cache cleaner is invoked. public void testCacheCleanupBasedOnZeroThreshold() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0%").build(); @@ -356,6 +357,42 @@ public void testCacheCleanupBasedOnZeroThreshold() throws Exception { IOUtils.close(secondReader); } + // when staleness count is higher than stale threshold, stale keys should be cleaned up. + public void testCacheCleanupBasedOnStaleThreshold_StalenessHigherThanThreshold() throws Exception { + threadPool = getThreadPool(); + Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.49").build(); + cache = getIndicesRequestCache(settings, threadPool); + + writer.addDocument(newDoc(0, "foo")); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + DirectoryReader secondReader = getReader(writer, indexShard.shardId()); + + // Get 2 entries into the cache + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); + assertEquals(1, cache.count()); + + cache.getOrCompute(getEntity(indexShard), getLoader(reader), secondReader, getTermBytes()); + assertEquals(2, cache.count()); + + // no stale keys so far + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + // Close the reader, to be enqueued for cleanup + reader.close(); + // 1 out of 2 keys ie 50% are now stale. + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); + // cache count should not be affected + assertEquals(2, cache.count()); + + // clean cache with 49% staleness threshold + cache.cacheCleanupManager.cleanCache(); + // cleanup should have taken effect with 49% threshold + assertEquals(1, cache.count()); + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + + IOUtils.close(secondReader); + } + + // when staleness count equal to stale threshold, stale keys should be cleaned up. public void testCacheCleanupBasedOnStaleThreshold_StalenessEqualToThreshold() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.5").build(); @@ -487,38 +524,53 @@ public void testStaleCount_WithoutReaderClosing_DecrementsStaleCount() throws Ex // test staleness count based on removal notifications public void testStaleCount_OnRemovalNotifications() throws Exception { threadPool = getThreadPool(); - Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.49").build(); + Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); cache = getIndicesRequestCache(settings, threadPool); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); - DirectoryReader secondReader = getReader(writer, indexShard.shardId()); - // Get 2 entries into the cache - cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); - assertEquals(1, cache.count()); - - cache.getOrCompute(getEntity(indexShard), getLoader(reader), secondReader, getTermBytes()); - assertEquals(2, cache.count()); - - // no stale keys so far + // Get 5 entries into the cache + int totalKeys = 5; + IndicesService.IndexShardCacheEntity entity = null; + TermQueryBuilder termQuery = null; + BytesReference termBytes = null; + for (int i = 1; i <= totalKeys; i++) { + termQuery = new TermQueryBuilder("id", "" + i); + termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + entity = new IndicesService.IndexShardCacheEntity(indexShard); + Loader loader = new Loader(reader, 0); + cache.getOrCompute(entity, loader, reader, termBytes); + assertEquals(i, cache.count()); + } + // no keys are stale yet assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); - // Close the reader, to be enqueued for cleanup + // closing the reader should make all keys stale reader.close(); - // 1 out of 2 keys ie 50% are now stale. - assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); - // cache count should not be affected - assertEquals(2, cache.count()); + assertEquals(totalKeys, cache.cacheCleanupManager.getStaleKeysCount().get()); - // clean cache with 49% staleness threshold - cache.cacheCleanupManager.cleanCache(); - // cleanup should have taken effect with 49% threshold - assertEquals(1, cache.count()); - assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + String readerCacheKeyId = getReaderCacheKeyId(reader); + IndicesRequestCache.Key key = new IndicesRequestCache.Key( + ((IndexShard) entity.getCacheIdentity()).shardId(), + termBytes, + readerCacheKeyId + ); - IOUtils.close(secondReader); + int staleCount = cache.cacheCleanupManager.getStaleKeysCount().get(); + // Notification for Replaced should not deduct the staleCount + cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.REPLACED)); + // stale keys count should stay the same + assertEquals(staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); + + // Notification for all but Replaced should deduct the staleCount + RemovalReason[] reasons = { RemovalReason.INVALIDATED, RemovalReason.EVICTED, RemovalReason.EXPLICIT, RemovalReason.CAPACITY }; + for (RemovalReason reason : reasons) { + cache.onRemoval(new RemovalNotification(key, termBytes, reason)); + assertEquals(--staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); + } } + // when staleness count less than the stale threshold, stale keys should NOT be cleaned up. public void testCacheCleanupBasedOnStaleThreshold_StalenessLesserThanThreshold() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "51%").build(); From 56f4e1f8d60c9819a1fb8dc0699a35a915e3d7b7 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Mon, 8 Apr 2024 11:41:29 -0700 Subject: [PATCH 13/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCacheTests.java | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 8c817d37ff470..161e5a8e60912 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -112,7 +112,7 @@ public void cleanup() throws IOException { public void testBasicOperationsCache() throws Exception { threadPool = getThreadPool(); - cache = getIndicesRequestCache(Settings.EMPTY, threadPool); + cache = getIndicesRequestCache(Settings.EMPTY); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); @@ -164,7 +164,7 @@ public void testBasicOperationsCache() throws Exception { public void testBasicOperationsCacheWithFeatureFlag() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.PLUGGABLE_CACHE, "true").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); @@ -215,7 +215,7 @@ public void testBasicOperationsCacheWithFeatureFlag() throws Exception { public void testCacheDifferentReaders() throws Exception { threadPool = getThreadPool(); - cache = getIndicesRequestCache(Settings.EMPTY, threadPool); + cache = getIndicesRequestCache(Settings.EMPTY); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); @@ -333,7 +333,7 @@ public void testCacheCleanupThresholdSettingValidator_Invalid_Percentage() { public void testCacheCleanupBasedOnZeroThreshold() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0%").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); DirectoryReader secondReader = getReader(writer, indexShard.shardId()); @@ -361,7 +361,7 @@ public void testCacheCleanupBasedOnZeroThreshold() throws Exception { public void testCacheCleanupBasedOnStaleThreshold_StalenessHigherThanThreshold() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.49").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); @@ -396,7 +396,7 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessHigherThanThreshold() public void testCacheCleanupBasedOnStaleThreshold_StalenessEqualToThreshold() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.5").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); DirectoryReader secondReader = getReader(writer, indexShard.shardId()); @@ -428,7 +428,7 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessEqualToThreshold() th public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); DirectoryReader secondReader = getReader(writer, indexShard.shardId()); @@ -462,7 +462,7 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStaleCount() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); DirectoryReader secondReader = getReader(writer, indexShard.shardId()); @@ -497,7 +497,7 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStal public void testStaleCount_WithoutReaderClosing_DecrementsStaleCount() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); @@ -525,7 +525,7 @@ public void testStaleCount_WithoutReaderClosing_DecrementsStaleCount() throws Ex public void testStaleCount_OnRemovalNotifications() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); @@ -574,7 +574,7 @@ public void testStaleCount_OnRemovalNotifications() throws Exception { public void testCacheCleanupBasedOnStaleThreshold_StalenessLesserThanThreshold() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "51%").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); @@ -607,7 +607,7 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessLesserThanThreshold() public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); - cache = getIndicesRequestCache(settings, threadPool); + cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); ShardId shardId = indexShard.shardId(); @@ -656,7 +656,7 @@ private DirectoryReader getReader(IndexWriter writer, ShardId shardId) throws IO return OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); } - private IndicesRequestCache getIndicesRequestCache(Settings settings, ThreadPool threadPool) { + private IndicesRequestCache getIndicesRequestCache(Settings settings){ IndicesService indicesService = getInstanceFromNode(IndicesService.class); return new IndicesRequestCache(settings, (shardId -> { IndexService indexService = null; @@ -692,7 +692,7 @@ public void testEviction() throws Exception { final ByteSizeValue size; { threadPool = getThreadPool(); - cache = getIndicesRequestCache(Settings.EMPTY, threadPool); + cache = getIndicesRequestCache(Settings.EMPTY); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); @@ -735,7 +735,7 @@ public void testEviction() throws Exception { public void testClearAllEntityIdentity() throws Exception { threadPool = getThreadPool(); - cache = getIndicesRequestCache(Settings.EMPTY, threadPool); + cache = getIndicesRequestCache(Settings.EMPTY); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); IndicesService.IndexShardCacheEntity entity = new IndicesService.IndexShardCacheEntity(indexShard); @@ -814,7 +814,7 @@ public BytesReference get() { public void testInvalidate() throws Exception { threadPool = getThreadPool(); - IndicesRequestCache cache = getIndicesRequestCache(Settings.EMPTY, threadPool); + IndicesRequestCache cache = getIndicesRequestCache(Settings.EMPTY); writer.addDocument(newDoc(0, "foo")); DirectoryReader reader = getReader(writer, indexShard.shardId()); From 6630cfcc4a7c4f6ea3d2c4a062e75bc1b3d72795 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Mon, 8 Apr 2024 14:00:10 -0700 Subject: [PATCH 14/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../java/org/opensearch/indices/IndicesRequestCacheTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 161e5a8e60912..2807293ed07d0 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -656,7 +656,7 @@ private DirectoryReader getReader(IndexWriter writer, ShardId shardId) throws IO return OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); } - private IndicesRequestCache getIndicesRequestCache(Settings settings){ + private IndicesRequestCache getIndicesRequestCache(Settings settings) { IndicesService indicesService = getInstanceFromNode(IndicesService.class); return new IndicesRequestCache(settings, (shardId -> { IndexService indexService = null; From 5de80d023788fe0078ce6625a0c56e5e9f779d76 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Mon, 8 Apr 2024 15:26:09 -0700 Subject: [PATCH 15/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 4ce94714d3bed..a5c1c91609b85 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -247,7 +247,15 @@ BytesReference getOrCompute( if (!registeredClosedListeners.containsKey(cleanupKey)) { Boolean previous = registeredClosedListeners.putIfAbsent(cleanupKey, Boolean.TRUE); if (previous == null) { - OpenSearchDirectoryReader.addReaderCloseListener(reader, cleanupKey); + try { + OpenSearchDirectoryReader.addReaderCloseListener(reader, cleanupKey); + } catch (Exception e) { + logger.warn("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 + // hence enqueuing it to clean-up + cacheCleanupManager.enqueueCleanupKey(cleanupKey); + throw e; + } } } cacheCleanupManager.updateCleanupKeyToCountMapOnCacheInsertion(cleanupKey); @@ -517,7 +525,13 @@ private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { * @param notification RemovalNotification of the cache entry evicted */ private void updateCleanupKeyCountOnKeyRemoval(CleanupKey cleanupKey, RemovalNotification notification) { - if (!notificationAffectsStaleness(notification) || stalenessThreshold == 0.0 || cleanupKey.entity == null) { + if (cleanupKey.entity == null) { + // this will only happen when the index/shard is deleted. + // we would have accounted this in staleKeysCount when the deletion of index/shard would have closed the associated reader + staleKeysCount.decrementAndGet(); + return; + } + if (!notificationAffectsStaleness(notification) || stalenessThreshold == 0.0) { return; } IndexShard indexShard = (IndexShard) cleanupKey.entity.getCacheIdentity(); From 139cbfc9f8631472b2ba8947971dc74c5d68bfdc Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Mon, 8 Apr 2024 15:26:17 -0700 Subject: [PATCH 16/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCacheTests.java | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 2807293ed07d0..df0e028f0245a 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -72,6 +72,7 @@ import org.opensearch.threadpool.ThreadPool; import org.junit.After; import org.junit.Before; +import org.junit.Test; import java.io.IOException; import java.util.ArrayList; @@ -83,6 +84,8 @@ import java.util.concurrent.atomic.AtomicInteger; import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -652,6 +655,56 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { assertEquals(0, cleanupKeyToCountMap.size()); } + // when registering a closed listener raises an exception, we should + @Test(expected = Exception.class) + public void testAddReaderCloseListenerRaisingException_shouldTrackStaleCountAppropriately() throws Exception { + IndexReader.CacheHelper cacheHelper = mock(IndexReader.CacheHelper.class); + doThrow(new Exception("Mock exception")).when(cacheHelper).addClosedListener(any()); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + + // assert there are no other entries + assertEquals(0, cache.count()); + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); + // the entry should be cached + assertEquals(1, cache.count()); + // cacheCleanupManager should have incremented the stale keys count + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); + // cacheCleanupManager should have removed the entry from the map + assertFalse(cache.cacheCleanupManager.getCleanupKeyToCountMap().containsKey(indexShard.shardId())); + + IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(reader)); + cache.onRemoval(new RemovalNotification(key, getTermBytes(), RemovalReason.INVALIDATED)); + // eviction of previous stale key from the cache should decrement staleKeysCount in iRC + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + } + + @Test(expected = Exception.class) + public void testAddReaderCloseListenerRaisingException_shouldCleanupCacheCorrectly() throws Exception { + IndexReader.CacheHelper cacheHelper = mock(IndexReader.CacheHelper.class); + doThrow(new Exception("Mock exception")).when(cacheHelper).addClosedListener(any()); + DirectoryReader reader = getReader(writer, indexShard.shardId()); + + // assert there are no other entries + assertEquals(0, cache.count()); + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + + cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); + // the entry should be cached + assertEquals(1, cache.count()); + // cacheCleanupManager should have incremented the stale keys count + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); + // cacheCleanupManager should have removed the entry from the map + assertFalse(cache.cacheCleanupManager.getCleanupKeyToCountMap().containsKey(indexShard.shardId())); + + cache.cacheCleanupManager.cleanCache(); + // cache cleaner should have cleaned it up + assertEquals(0, cache.count()); + // stale keys count should ne decremented. + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + } + private DirectoryReader getReader(IndexWriter writer, ShardId shardId) throws IOException { return OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), shardId); } From eaeba38c91fc4392495241d0f312ff3ba1167661 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Mon, 8 Apr 2024 23:05:58 -0700 Subject: [PATCH 17/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../java/org/opensearch/indices/IndicesRequestCacheTests.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index df0e028f0245a..2ab38faba6d9e 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -45,6 +45,7 @@ import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; import org.opensearch.common.CheckedSupplier; +import org.opensearch.common.SuppressForbidden; import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.module.CacheModule; @@ -657,6 +658,7 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { // when registering a closed listener raises an exception, we should @Test(expected = Exception.class) + @SuppressForbidden(reason = "only way to avoid registering a reader after caching a key") public void testAddReaderCloseListenerRaisingException_shouldTrackStaleCountAppropriately() throws Exception { IndexReader.CacheHelper cacheHelper = mock(IndexReader.CacheHelper.class); doThrow(new Exception("Mock exception")).when(cacheHelper).addClosedListener(any()); @@ -681,6 +683,7 @@ public void testAddReaderCloseListenerRaisingException_shouldTrackStaleCountAppr } @Test(expected = Exception.class) + @SuppressForbidden(reason = "only way to avoid registering a reader after caching a key") public void testAddReaderCloseListenerRaisingException_shouldCleanupCacheCorrectly() throws Exception { IndexReader.CacheHelper cacheHelper = mock(IndexReader.CacheHelper.class); doThrow(new Exception("Mock exception")).when(cacheHelper).addClosedListener(any()); From 6122aa2dd130b49b28ce386667b41fcce78d2a0d Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Tue, 9 Apr 2024 16:34:06 -0700 Subject: [PATCH 18/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index a5c1c91609b85..21fcf1496e51f 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -240,27 +240,31 @@ 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); - 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) { - try { + 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); - } catch (Exception e) { - logger.warn("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 - // hence enqueuing it to clean-up - cacheCleanupManager.enqueueCleanupKey(cleanupKey); - throw e; } } + 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 + // hence enqueuing it to clean-up cacheCleanupManager.updateCleanupKeyToCountMapOnCacheInsertion(cleanupKey); - } else { - cacheEntity.onHit(); + cacheCleanupManager.enqueueCleanupKey(cleanupKey); + throw e; } return value; } From e870a9836dfe2845ab1d81477a691133238d36d1 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Tue, 9 Apr 2024 16:34:16 -0700 Subject: [PATCH 19/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCacheTests.java | 101 ++++++++++-------- 1 file changed, 54 insertions(+), 47 deletions(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 2ab38faba6d9e..7499c4a784a73 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -42,10 +42,10 @@ 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; -import org.opensearch.common.SuppressForbidden; import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.module.CacheModule; @@ -73,7 +73,6 @@ import org.opensearch.threadpool.ThreadPool; import org.junit.After; import org.junit.Before; -import org.junit.Test; import java.io.IOException; import java.util.ArrayList; @@ -84,9 +83,9 @@ import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; +import static org.hamcrest.Matchers.instanceOf; +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.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -656,56 +655,34 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { assertEquals(0, cleanupKeyToCountMap.size()); } - // when registering a closed listener raises an exception, we should - @Test(expected = Exception.class) - @SuppressForbidden(reason = "only way to avoid registering a reader after caching a key") - public void testAddReaderCloseListenerRaisingException_shouldTrackStaleCountAppropriately() throws Exception { - IndexReader.CacheHelper cacheHelper = mock(IndexReader.CacheHelper.class); - doThrow(new Exception("Mock exception")).when(cacheHelper).addClosedListener(any()); - DirectoryReader reader = getReader(writer, indexShard.shardId()); - - // assert there are no other entries - assertEquals(0, cache.count()); - assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); - - cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); - // the entry should be cached - assertEquals(1, cache.count()); - // cacheCleanupManager should have incremented the stale keys count - assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); - // cacheCleanupManager should have removed the entry from the map - assertFalse(cache.cacheCleanupManager.getCleanupKeyToCountMap().containsKey(indexShard.shardId())); - - IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(reader)); - cache.onRemoval(new RemovalNotification(key, getTermBytes(), RemovalReason.INVALIDATED)); - // eviction of previous stale key from the cache should decrement staleKeysCount in iRC - assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); - } - - @Test(expected = Exception.class) - @SuppressForbidden(reason = "only way to avoid registering a reader after caching a key") - public void testAddReaderCloseListenerRaisingException_shouldCleanupCacheCorrectly() throws Exception { - IndexReader.CacheHelper cacheHelper = mock(IndexReader.CacheHelper.class); - doThrow(new Exception("Mock exception")).when(cacheHelper).addClosedListener(any()); - DirectoryReader reader = getReader(writer, indexShard.shardId()); + // test the cleanupKeyToCountMap are set appropriately when readers are closed + 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); - // assert there are no other entries - assertEquals(0, cache.count()); - assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + writer.addDocument(newDoc(0, "foo")); + ShardId shardId = indexShard.shardId(); + DirectoryReader reader = getReader(writer, shardId); - cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); - // the entry should be cached + assertTrue(cache.cacheCleanupManager.getCleanupKeyToCountMap().isEmpty()); + try { + cache.getOrCompute(getEntity(indexShard), getAutoCloseLoader(reader), reader, getTermBytes()); + } catch (Exception e) { + assertThat(e, instanceOf(AlreadyClosedException.class)); + } assertEquals(1, cache.count()); - // cacheCleanupManager should have incremented the stale keys count assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); - // cacheCleanupManager should have removed the entry from the map - assertFalse(cache.cacheCleanupManager.getCleanupKeyToCountMap().containsKey(indexShard.shardId())); + assertTrue(cache.cacheCleanupManager.getCleanupKeyToCountMap().isEmpty()); cache.cacheCleanupManager.cleanCache(); - // cache cleaner should have cleaned it up + assertEquals(0, cache.count()); - // stale keys count should ne decremented. assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + assertTrue(cache.cacheCleanupManager.getCleanupKeyToCountMap().isEmpty()); } private DirectoryReader getReader(IndexWriter writer, ShardId shardId) throws IOException { @@ -729,6 +706,10 @@ 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); } @@ -842,7 +823,7 @@ public Iterable newDoc(int id, String value) { private static class Loader implements CheckedSupplier { - private final DirectoryReader reader; + final DirectoryReader reader; private final int id; public boolean loadedFromCache = true; @@ -868,6 +849,32 @@ 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); From bed1121f5638e78c1017cc4cb843fd3c430bffd8 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Tue, 9 Apr 2024 16:41:37 -0700 Subject: [PATCH 20/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../main/java/org/opensearch/indices/IndicesRequestCache.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 21fcf1496e51f..c18cc87bc700e 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -530,8 +530,8 @@ private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { */ private void updateCleanupKeyCountOnKeyRemoval(CleanupKey cleanupKey, RemovalNotification notification) { if (cleanupKey.entity == null) { - // this will only happen when the index/shard is deleted. - // we would have accounted this in staleKeysCount when the deletion of index/shard would have closed the associated reader + // this will only happen when the shard is deleted. + // we would have accounted this in staleKeysCount when the deletion of shard would have closed the associated readers staleKeysCount.decrementAndGet(); return; } From e178ea7148844cdfd4d68d9c0070e24274089cef Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Tue, 9 Apr 2024 16:41:42 -0700 Subject: [PATCH 21/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../java/org/opensearch/indices/IndicesRequestCacheTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 7499c4a784a73..af08851917295 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -83,9 +83,9 @@ import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; -import static org.hamcrest.Matchers.instanceOf; 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; From 3a2e8520de0f246a31e120221d83a78a296a058d Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Wed, 10 Apr 2024 10:13:48 -0700 Subject: [PATCH 22/37] code comments only Signed-off-by: Kiran Prakash --- .../java/org/opensearch/indices/IndicesRequestCache.java | 4 ++-- .../org/opensearch/indices/IndicesRequestCacheTests.java | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index c18cc87bc700e..ad6fe1242b941 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -260,8 +260,8 @@ BytesReference getOrCompute( 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 - // hence enqueuing it to clean-up + // 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; diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index af08851917295..faa03cd6706ad 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -655,7 +655,9 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { assertEquals(0, cleanupKeyToCountMap.size()); } - // test the cleanupKeyToCountMap are set appropriately when readers are closed + // 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() @@ -670,6 +672,7 @@ public void testCleanupKeyToOnClosedReader_cleansupStalenessAsExpected() throws 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)); From 5989e1d58423576e388b2dbb7177ec13848c79ce Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Wed, 10 Apr 2024 10:23:56 -0700 Subject: [PATCH 23/37] docs changes Signed-off-by: Kiran Prakash --- .../java/org/opensearch/indices/IndicesRequestCache.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index ad6fe1242b941..d65a20b033f52 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -530,8 +530,10 @@ private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { */ private void updateCleanupKeyCountOnKeyRemoval(CleanupKey cleanupKey, RemovalNotification notification) { if (cleanupKey.entity == null) { - // this will only happen when the shard is deleted. - // we would have accounted this in staleKeysCount when the deletion of shard would have closed the associated readers + /* + * on shard close, the shard is still lying around so this will only happen when the shard is deleted. + * we would have accounted this in staleKeysCount when the deletion of shard would have closed the associated readers + * */ staleKeysCount.decrementAndGet(); return; } From 68b949b8beaa65e76253ee9b73b6dc5b1f6086b8 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Wed, 10 Apr 2024 12:09:21 -0700 Subject: [PATCH 24/37] Update CHANGELOG.md Signed-off-by: Kiran Prakash --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b81d017f7810..f29b6686b8626 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix bulk API ignores ingest pipeline for upsert ([#12883](https://github.com/opensearch-project/OpenSearch/pull/12883)) - Fix issue with feature flags where default value may not be honored ([#12849](https://github.com/opensearch-project/OpenSearch/pull/12849)) - Fix UOE While building Exists query for nested search_as_you_type field ([#12048](https://github.com/opensearch-project/OpenSearch/pull/12048)) -- Client with Java 8 runtime and Apache HttpClient 5 Transport fails with java.lang.NoSuchMethodError: java.nio.ByteBuffer.flip()Ljava/nio/ByteBuffer ([#13100](https://github.com/opensearch-project/opensearch-java/pull/13100)) - Client with Java 8 runtime and Apache HttpClient 5 Transport fails with java.lang.NoSuchMethodError: java.nio.ByteBuffer.flip()Ljava/nio/ByteBuffer ([#13100](https://github.com/opensearch-project/opensearch-java/pull/13100)) - Fix implement mark() and markSupported() in class FilterStreamInput ([#13098](https://github.com/opensearch-project/OpenSearch/pull/13098)) - Fix IndicesRequestCache Stale calculation to be accurate. ([#13070](https://github.com/opensearch-project/OpenSearch/pull/13070)] From dd9161343930cb022a14faa1293de484070c043c Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 11 Apr 2024 13:26:34 -0700 Subject: [PATCH 25/37] revert catching AlreadyClosedException Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 32 +++------ .../indices/IndicesRequestCacheTests.java | 66 ------------------- 2 files changed, 10 insertions(+), 88 deletions(-) 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); From 707f8bdd4a64b2df7a463dd50f4d078c0c056369 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 11 Apr 2024 15:44:30 -0700 Subject: [PATCH 26/37] assert Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 38 ++++++++++--------- .../indices/IndicesRequestCacheTests.java | 4 -- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 7fc4fcbdc5cec..8f45af73028f1 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -519,9 +519,9 @@ private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { private void updateCleanupKeyCountOnKeyRemoval(CleanupKey cleanupKey, RemovalNotification notification) { if (cleanupKey.entity == null) { /* - * on shard close, the shard is still lying around so this will only happen when the shard is deleted. - * we would have accounted this in staleKeysCount when the deletion of shard would have closed the associated readers - * */ + * on shard close, the shard is still lying around so this will only happen when the shard is deleted. + * we would have accounted this in staleKeysCount when the deletion of shard would have closed the associated readers + * */ staleKeysCount.decrementAndGet(); return; } @@ -537,26 +537,28 @@ private void updateCleanupKeyCountOnKeyRemoval(CleanupKey cleanupKey, RemovalNot cleanupKeyToCountMap.compute(shardId, (key, readerCacheKeyMap) -> { if (readerCacheKeyMap == null || !readerCacheKeyMap.containsKey(cleanupKey.readerCacheKeyId)) { - // If ShardId is not present or readerCacheKeyId is not present, decrement staleKeysCount + // If ShardId is not present or readerCacheKeyId is not present + // it should have already been accounted for and hence been removed from this map + // so decrement staleKeysCount staleKeysCount.decrementAndGet(); - return null; // Returning null removes the entry for the shardId, if it exists + // Returning null removes the entry for the shardId, if it exists + return null; } else { - // Proceed to adjust the count for the readerCacheKeyId + // If it is in the map, it is not stale yet. + // Proceed to adjust the count for the readerCacheKeyId in the map + // but do not decrement the staleKeysCount Integer count = readerCacheKeyMap.get(cleanupKey.readerCacheKeyId); - if (count == null) { - staleKeysCount.decrementAndGet(); + // this should never be null + assert (count != null); + // Reduce the count by 1 + int newCount = count - 1; + if (newCount <= 0) { + // Remove the readerCacheKeyId entry if new count is zero or less + readerCacheKeyMap.remove(cleanupKey.readerCacheKeyId); } else { - // Reduce the count by 1 - int newCount = count - 1; - if (newCount <= 0) { - // Remove the readerCacheKeyId entry if new count is zero or less - readerCacheKeyMap.remove(cleanupKey.readerCacheKeyId); - } else { - // Update the map with the new count - readerCacheKeyMap.put(cleanupKey.readerCacheKeyId, newCount); - } + // Update the map with the new count + readerCacheKeyMap.put(cleanupKey.readerCacheKeyId, newCount); } - // If after modification, the readerCacheKeyMap is empty, we return null to remove the ShardId entry return readerCacheKeyMap.isEmpty() ? null : readerCacheKeyMap; } diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index be9cc1ea7905a..517a401a88094 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -632,20 +632,16 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); // cache count should not be affected assertEquals(2, cache.count()); - // test the mapping assertFalse(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(reader))); - // second reader's mapping should not be affected assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(secondReader))); - // Close the second reader secondReader.close(); // both keys should now be stale assertEquals(2, cache.cacheCleanupManager.getStaleKeysCount().get()); // cache count should not be affected assertEquals(2, cache.count()); - // test the mapping // since all the readers of this shard is closed, // the cleanupKeyToCountMap should have no entries From 70cc9901fe5a9c6a828d49a11a0542bbc31133e1 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 11 Apr 2024 18:14:49 -0700 Subject: [PATCH 27/37] conflicts Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 29 ++++++++----------- .../indices/IndicesRequestCacheTests.java | 2 +- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index c61371904c35a..53796ea19451b 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -212,19 +212,7 @@ public void onRemoval(RemovalNotification notification) { Key key = notification.getKey(); cacheEntityLookup.apply(key.shardId).ifPresent(entity -> entity.onRemoval(notification)); CleanupKey cleanupKey = new CleanupKey(cacheEntityLookup.apply(key.shardId).orElse(null), key.readerCacheKeyId); - cacheCleanupManager.updateCleanupKeyCountOnKeyRemoval(cleanupKey, notification); - } - - /** - * This method checks if the removal reason of the notification is not REPLACED. - * The reason of the notification is REPLACED when a cache entry's value is updated, since replacing an entry - * does not affect the staleness count, we skip such notifications. - * - *

If the removal reason is anything other than REPLACED, this will return true. - * If the removal reason is REPLACED, this will return false. - */ - private boolean notificationAffectsStaleness(RemovalNotification notification) { - return notification.getRemovalReason() != RemovalReason.REPLACED; + cacheCleanupManager.updateStaleCountOnEntryRemoval(cleanupKey, notification); } BytesReference getOrCompute( @@ -243,6 +231,8 @@ 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); + IndexShard indexShard = (IndexShard) cacheEntity.getCacheIdentity(); + indexShard.refresh("test"); if (cacheLoader.isLoaded()) { cacheEntity.onMiss(); // see if it's the first time we see this reader, and make sure to register a cleanup key @@ -253,10 +243,11 @@ BytesReference getOrCompute( OpenSearchDirectoryReader.addReaderCloseListener(reader, cleanupKey); } } - cacheCleanupManager.updateCleanupKeyToCountMapOnCacheInsertion(cleanupKey); + cacheCleanupManager.updateStaleCountOnCacheInsert(cleanupKey); } else { cacheEntity.onHit(); } + return value; } @@ -489,7 +480,7 @@ void enqueueCleanupKey(CleanupKey cleanupKey) { * * @param cleanupKey the CleanupKey to be updated in the map */ - private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { + private void updateStaleCountOnCacheInsert(CleanupKey cleanupKey) { if (stalenessThreshold == 0.0 || cleanupKey.entity == null) { return; } @@ -519,7 +510,7 @@ private void updateCleanupKeyToCountMapOnCacheInsertion(CleanupKey cleanupKey) { * @param cleanupKey the CleanupKey that has been evicted from the cache * @param notification RemovalNotification of the cache entry evicted */ - private void updateCleanupKeyCountOnKeyRemoval(CleanupKey cleanupKey, RemovalNotification notification) { + private void updateStaleCountOnEntryRemoval(CleanupKey cleanupKey, RemovalNotification notification) { if (cleanupKey.entity == null) { /* * on shard close, the shard is still lying around so this will only happen when the shard is deleted. @@ -528,7 +519,11 @@ private void updateCleanupKeyCountOnKeyRemoval(CleanupKey cleanupKey, RemovalNot staleKeysCount.decrementAndGet(); return; } - if (!notificationAffectsStaleness(notification) || stalenessThreshold == 0.0) { + /* + * The reason of the notification is REPLACED when a cache entry's value is updated, since replacing an entry + * does not affect the staleness count, we skip such notifications. + * */ + if (notification.getRemovalReason() == RemovalReason.REPLACED) { return; } IndexShard indexShard = (IndexShard) cleanupKey.entity.getCacheIdentity(); diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 1b9af641ab90e..7b40af5cf030e 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -663,7 +663,7 @@ private IndicesRequestCache getIndicesRequestCache(Settings settings) { return Optional.empty(); } return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool); + }), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool, ClusterServiceUtils.createClusterService(threadPool)); } private Loader getLoader(DirectoryReader reader) { From b6b5f2b15c25ffc7cd25a588cb60a3266d39c4e9 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 11 Apr 2024 18:16:16 -0700 Subject: [PATCH 28/37] Update IndicesRequestCacheTests.java Signed-off-by: Kiran Prakash --- .../org/opensearch/indices/IndicesRequestCacheTests.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 7b40af5cf030e..7d1ac92746a03 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -663,7 +663,11 @@ private IndicesRequestCache getIndicesRequestCache(Settings settings) { return Optional.empty(); } return Optional.of(new IndicesService.IndexShardCacheEntity(indexService.getShard(shardId.id()))); - }), new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool, ClusterServiceUtils.createClusterService(threadPool)); + }), + new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), + threadPool, + ClusterServiceUtils.createClusterService(threadPool) + ); } private Loader getLoader(DirectoryReader reader) { From 4c691871def27add123124c4a8c49516dfbd58c9 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Thu, 11 Apr 2024 18:23:50 -0700 Subject: [PATCH 29/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../main/java/org/opensearch/indices/IndicesRequestCache.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 53796ea19451b..298e919a96e01 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -231,8 +231,6 @@ 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); - IndexShard indexShard = (IndexShard) cacheEntity.getCacheIdentity(); - indexShard.refresh("test"); if (cacheLoader.isLoaded()) { cacheEntity.onMiss(); // see if it's the first time we see this reader, and make sure to register a cleanup key From 31d9c54cab740c364d446db10930718c4bafe0ec Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Fri, 12 Apr 2024 07:17:48 -0700 Subject: [PATCH 30/37] address comments Signed-off-by: Kiran Prakash --- CHANGELOG.md | 2 +- .../main/java/org/opensearch/indices/IndicesRequestCache.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16ca798177201..000c38b8d8155 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,7 +48,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix UOE While building Exists query for nested search_as_you_type field ([#12048](https://github.com/opensearch-project/OpenSearch/pull/12048)) - Client with Java 8 runtime and Apache HttpClient 5 Transport fails with java.lang.NoSuchMethodError: java.nio.ByteBuffer.flip()Ljava/nio/ByteBuffer ([#13100](https://github.com/opensearch-project/opensearch-java/pull/13100)) - Fix implement mark() and markSupported() in class FilterStreamInput ([#13098](https://github.com/opensearch-project/OpenSearch/pull/13098)) -- Fix IndicesRequestCache Stale calculation to be accurate. ([#13070](https://github.com/opensearch-project/OpenSearch/pull/13070)] +- Fix IndicesRequestCache Stale calculation ([#13070](https://github.com/opensearch-project/OpenSearch/pull/13070)] ### Security diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 298e919a96e01..c8e8fb10efd2e 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -545,7 +545,7 @@ private void updateStaleCountOnEntryRemoval(CleanupKey cleanupKey, RemovalNotifi // but do not decrement the staleKeysCount Integer count = readerCacheKeyMap.get(cleanupKey.readerCacheKeyId); // this should never be null - assert (count != null); + assert (count != null && count >= 0); // Reduce the count by 1 int newCount = count - 1; if (newCount <= 0) { From 9aeb82d641220dfeae46d103a47575cc10c4cc0c Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Fri, 12 Apr 2024 08:43:50 -0700 Subject: [PATCH 31/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index c8e8fb10efd2e..3ee57fdb7efff 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -509,19 +509,15 @@ private void updateStaleCountOnCacheInsert(CleanupKey cleanupKey) { * @param notification RemovalNotification of the cache entry evicted */ private void updateStaleCountOnEntryRemoval(CleanupKey cleanupKey, RemovalNotification notification) { - if (cleanupKey.entity == null) { - /* - * on shard close, the shard is still lying around so this will only happen when the shard is deleted. - * we would have accounted this in staleKeysCount when the deletion of shard would have closed the associated readers - * */ - staleKeysCount.decrementAndGet(); + if (notification.getRemovalReason() == RemovalReason.REPLACED) { + // The reason of the notification is REPLACED when a cache entry's value is updated, since replacing an entry + // does not affect the staleness count, we skip such notifications. return; } - /* - * The reason of the notification is REPLACED when a cache entry's value is updated, since replacing an entry - * does not affect the staleness count, we skip such notifications. - * */ - if (notification.getRemovalReason() == RemovalReason.REPLACED) { + if (cleanupKey.entity == null) { + // on shard close, the shard is still lying around so this will only happen when the shard is deleted. + // we would have accounted this in staleKeysCount when the deletion of shard would have closed the associated readers + staleKeysCount.decrementAndGet(); return; } IndexShard indexShard = (IndexShard) cleanupKey.entity.getCacheIdentity(); From 8bfa6c4d9d59f1b608ce525090de7740cb027bdb Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Fri, 12 Apr 2024 10:34:01 -0700 Subject: [PATCH 32/37] Update IndicesRequestCache.java Signed-off-by: Kiran Prakash --- .../java/org/opensearch/indices/IndicesRequestCache.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 3ee57fdb7efff..d4ea8fc0c2c88 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -515,8 +515,9 @@ private void updateStaleCountOnEntryRemoval(CleanupKey cleanupKey, RemovalNotifi return; } if (cleanupKey.entity == null) { - // on shard close, the shard is still lying around so this will only happen when the shard is deleted. - // we would have accounted this in staleKeysCount when the deletion of shard would have closed the associated readers + // entity will only be null when the shard is closed/deleted + // we would have accounted this in staleKeysCount when the closing/deletion of shard would have closed the associated + // readers staleKeysCount.decrementAndGet(); return; } From d5cdf5099a01a102031e8a5da6f1e740e238df3e Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Tue, 23 Apr 2024 11:33:21 -0700 Subject: [PATCH 33/37] address conflicts Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 4 +- .../indices/IndicesRequestCacheTests.java | 39 +++++++++++++------ 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index cb641b587f59b..58446f4226857 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -224,9 +224,9 @@ public void onRemoval(RemovalNotification, BytesReference> notifi notification.getValue(), notification.getRemovalReason() ); - cacheEntityLookup.apply(key.shardId).ifPresent(entity -> entity.onRemoval(notification)); + cacheEntityLookup.apply(key.shardId).ifPresent(entity -> entity.onRemoval(newNotification)); CleanupKey cleanupKey = new CleanupKey(cacheEntityLookup.apply(key.shardId).orElse(null), key.readerCacheKeyId); - cacheCleanupManager.updateStaleCountOnEntryRemoval(cleanupKey, notification); + cacheCleanupManager.updateStaleCountOnEntryRemoval(cleanupKey, newNotification); } private ICacheKey getICacheKey(Key key) { diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 55f25ae8a8065..203b0048b09ec 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -50,6 +50,7 @@ import org.opensearch.common.cache.RemovalNotification; import org.opensearch.common.cache.RemovalReason; import org.opensearch.common.cache.module.CacheModule; +import org.opensearch.common.cache.stats.ImmutableCacheStats; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.lucene.index.OpenSearchDirectoryReader; import org.opensearch.common.settings.Settings; @@ -90,6 +91,7 @@ import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; public class IndicesRequestCacheTests extends OpenSearchSingleNodeTestCase { private ThreadPool threadPool; @@ -459,11 +461,10 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( cache.onRemoval( new RemovalNotification, BytesReference>( new ICacheKey<>(key), - termBytes, + getTermBytes(), RemovalReason.EVICTED ) ); - staleKeysCount = cache.cacheCleanupManager.getStaleKeysCount(); // eviction of previous stale key from the cache should decrement staleKeysCount in iRC assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); @@ -500,7 +501,7 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStal cache.onRemoval( new RemovalNotification, BytesReference>( new ICacheKey<>(key), - termBytes, + getTermBytes(), RemovalReason.EVICTED ) ); @@ -532,7 +533,12 @@ public void testStaleCount_WithoutReaderClosing_DecrementsStaleCount() throws Ex assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); // create notification for removal of non-stale entry IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(reader)); - cache.onRemoval(new RemovalNotification(key, getTermBytes(), RemovalReason.EVICTED)); + cache.onRemoval( + new RemovalNotification, BytesReference>( + new ICacheKey<>(key), + getTermBytes(), + RemovalReason.EVICTED + )); // stale keys count should stay zero assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); @@ -576,14 +582,24 @@ public void testStaleCount_OnRemovalNotifications() throws Exception { int staleCount = cache.cacheCleanupManager.getStaleKeysCount().get(); // Notification for Replaced should not deduct the staleCount - cache.onRemoval(new RemovalNotification(key, termBytes, RemovalReason.REPLACED)); + cache.onRemoval( + new RemovalNotification, BytesReference>( + new ICacheKey<>(key), + getTermBytes(), + RemovalReason.REPLACED + )); // stale keys count should stay the same assertEquals(staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); // Notification for all but Replaced should deduct the staleCount RemovalReason[] reasons = { RemovalReason.INVALIDATED, RemovalReason.EVICTED, RemovalReason.EXPLICIT, RemovalReason.CAPACITY }; for (RemovalReason reason : reasons) { - cache.onRemoval(new RemovalNotification(key, termBytes, reason)); + cache.onRemoval( + new RemovalNotification, BytesReference>( + new ICacheKey<>(key), + getTermBytes(), + reason + )); assertEquals(--staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); } } @@ -713,6 +729,8 @@ public void testClosingIndexWipesStats() throws Exception { Settings indexSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numShards).build(); String indexToKeepName = "test"; String indexToCloseName = "test2"; + // delete all indices if already + assertAcked(client().admin().indices().prepareDelete("_all").get()); IndexService indexToKeep = createIndex(indexToKeepName, indexSettings); IndexService indexToClose = createIndex(indexToCloseName, indexSettings); for (int i = 0; i < numShards; i++) { @@ -720,9 +738,9 @@ public void testClosingIndexWipesStats() throws Exception { assertNotNull(indexToKeep.getShard(i)); assertNotNull(indexToClose.getShard(i)); } - ThreadPool threadPool = getThreadPool(); + threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.001%").build(); - IndicesRequestCache cache = new IndicesRequestCache(settings, (shardId -> { + cache = new IndicesRequestCache(settings, (shardId -> { IndexService indexService = null; try { indexService = indicesService.indexServiceSafe(shardId.getIndex()); @@ -739,8 +757,6 @@ public void testClosingIndexWipesStats() throws Exception { threadPool, ClusterServiceUtils.createClusterService(threadPool) ); - Directory dir = newDirectory(); - IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); writer.addDocument(newDoc(0, "foo")); TermQueryBuilder termQuery = new TermQueryBuilder("id", "0"); @@ -813,8 +829,7 @@ public void testClosingIndexWipesStats() throws Exception { for (DirectoryReader reader : readersToKeep) { IOUtils.close(reader); } - IOUtils.close(secondReader, writer, dir, cache); - terminate(threadPool); + IOUtils.close(secondReader); } public void testEviction() throws Exception { From 90682276278c469b66f77f0d3a1e90af1c3b714b Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Tue, 23 Apr 2024 11:34:53 -0700 Subject: [PATCH 34/37] spotless apply Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCacheTests.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index 203b0048b09ec..b927e1b3f9962 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -89,9 +89,9 @@ import java.util.concurrent.atomic.AtomicInteger; import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; public class IndicesRequestCacheTests extends OpenSearchSingleNodeTestCase { private ThreadPool threadPool; @@ -538,7 +538,8 @@ public void testStaleCount_WithoutReaderClosing_DecrementsStaleCount() throws Ex new ICacheKey<>(key), getTermBytes(), RemovalReason.EVICTED - )); + ) + ); // stale keys count should stay zero assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); @@ -587,7 +588,8 @@ public void testStaleCount_OnRemovalNotifications() throws Exception { new ICacheKey<>(key), getTermBytes(), RemovalReason.REPLACED - )); + ) + ); // stale keys count should stay the same assertEquals(staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); @@ -595,11 +597,8 @@ public void testStaleCount_OnRemovalNotifications() throws Exception { RemovalReason[] reasons = { RemovalReason.INVALIDATED, RemovalReason.EVICTED, RemovalReason.EXPLICIT, RemovalReason.CAPACITY }; for (RemovalReason reason : reasons) { cache.onRemoval( - new RemovalNotification, BytesReference>( - new ICacheKey<>(key), - getTermBytes(), - reason - )); + new RemovalNotification, BytesReference>(new ICacheKey<>(key), getTermBytes(), reason) + ); assertEquals(--staleCount, cache.cacheCleanupManager.getStaleKeysCount().get()); } } From 5a15c874d95c428ff4a5191ac31aa6c5be357739 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Tue, 23 Apr 2024 12:54:53 -0700 Subject: [PATCH 35/37] address comments Signed-off-by: Kiran Prakash --- .../main/java/org/opensearch/indices/IndicesRequestCache.java | 2 +- .../java/org/opensearch/indices/IndicesRequestCacheTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 58446f4226857..e3f0b3b504db6 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -563,7 +563,7 @@ private void updateStaleCountOnEntryRemoval(CleanupKey cleanupKey, RemovalNotifi // it should have already been accounted for and hence been removed from this map // so decrement staleKeysCount staleKeysCount.decrementAndGet(); - // Returning null removes the entry for the shardId, if it exists + // Returning the current value null return null; } else { // If it is in the map, it is not stale yet. diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index b927e1b3f9962..e9235941396e6 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -378,7 +378,7 @@ public void testCacheCleanupBasedOnStaleThreshold_StalenessHigherThanThreshold() cache.getOrCompute(getEntity(indexShard), getLoader(reader), reader, getTermBytes()); assertEquals(1, cache.count()); - cache.getOrCompute(getEntity(indexShard), getLoader(reader), secondReader, getTermBytes()); + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes()); assertEquals(2, cache.count()); // no stale keys so far From 430e57c884dd7caea0ea85ee615a5c5dbe1c7a0c Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Tue, 23 Apr 2024 13:59:58 -0700 Subject: [PATCH 36/37] update code comments Signed-off-by: Kiran Prakash --- .../org/opensearch/indices/IndicesRequestCache.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index e3f0b3b504db6..3cc8ef1454633 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -524,15 +524,13 @@ private void updateStaleCountOnCacheInsert(CleanupKey cleanupKey) { } /** - * Updates the cleanupKeyToCountMap and staleKeysCount when a cache eviction occurs. + * Handles the eviction of a cache entry. * *

This method is called when an entry is evicted from the cache. - * It decrements the count of the entry in the cleanupKeyToCountMap. - * It also decrements the staleKeysCount only if the entry was accounted. - * If the count of the CleanupKey becomes zero, it removes the CleanupKey from the map. - * - *

We update the cleanupKeyToCountMap on every key removed from cache - * except for the keys removed with reason Replaced + * We consider all removal notifications except with the reason Replaced + * {@link #incrementStaleKeysCount} would have removed the entries from the map and increment the {@link #staleKeysCount} + * Hence we decrement {@link #staleKeysCount} if we do not find the shardId or readerCacheKeyId in the map. + * Skip decrementing staleKeysCount if we find the shardId or readerCacheKeyId in the map since it would have not been accounted for in the staleKeysCount in * * @param cleanupKey the CleanupKey that has been evicted from the cache * @param notification RemovalNotification of the cache entry evicted From 581ea2afdbb3d3618e31fd517c06ff2880bf6ff7 Mon Sep 17 00:00:00 2001 From: Kiran Prakash Date: Wed, 24 Apr 2024 10:01:15 -0700 Subject: [PATCH 37/37] address bug & add tests Signed-off-by: Kiran Prakash --- .../indices/IndicesRequestCache.java | 12 +- .../indices/IndicesRequestCacheTests.java | 103 +++++++++++++++--- 2 files changed, 95 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java index 3cc8ef1454633..039e14a031f3f 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -561,8 +561,8 @@ private void updateStaleCountOnEntryRemoval(CleanupKey cleanupKey, RemovalNotifi // it should have already been accounted for and hence been removed from this map // so decrement staleKeysCount staleKeysCount.decrementAndGet(); - // Returning the current value null - return null; + // Return the current map + return readerCacheKeyMap; } else { // If it is in the map, it is not stale yet. // Proceed to adjust the count for the readerCacheKeyId in the map @@ -572,12 +572,12 @@ private void updateStaleCountOnEntryRemoval(CleanupKey cleanupKey, RemovalNotifi assert (count != null && count >= 0); // Reduce the count by 1 int newCount = count - 1; - if (newCount <= 0) { - // Remove the readerCacheKeyId entry if new count is zero or less - readerCacheKeyMap.remove(cleanupKey.readerCacheKeyId); - } else { + if (newCount > 0) { // Update the map with the new count readerCacheKeyMap.put(cleanupKey.readerCacheKeyId, newCount); + } else { + // Remove the readerCacheKeyId entry if new count is zero + readerCacheKeyMap.remove(cleanupKey.readerCacheKeyId); } // If after modification, the readerCacheKeyMap is empty, we return null to remove the ShardId entry return readerCacheKeyMap.isEmpty() ? null : readerCacheKeyMap; diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index e9235941396e6..051acfe9d085a 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -437,6 +437,7 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); + ShardId shardId = indexShard.shardId(); DirectoryReader reader = getReader(writer, indexShard.shardId()); DirectoryReader secondReader = getReader(writer, indexShard.shardId()); @@ -457,6 +458,14 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( assertEquals(2, cache.count()); IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(reader)); + // test the mapping + ConcurrentMap> cleanupKeyToCountMap = cache.cacheCleanupManager.getCleanupKeyToCountMap(); + // shard id should exist + assertTrue(cleanupKeyToCountMap.containsKey(shardId)); + // reader CacheKeyId should NOT exist + assertFalse(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(reader))); + // secondReader CacheKeyId should exist + assertTrue(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(secondReader))); cache.onRemoval( new RemovalNotification, BytesReference>( @@ -465,6 +474,14 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( RemovalReason.EVICTED ) ); + + // test the mapping, it should stay the same + // shard id should exist + assertTrue(cleanupKeyToCountMap.containsKey(shardId)); + // reader CacheKeyId should NOT exist + assertFalse(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(reader))); + // secondReader CacheKeyId should exist + assertTrue(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(secondReader))); // eviction of previous stale key from the cache should decrement staleKeysCount in iRC assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); @@ -472,11 +489,12 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DecrementsStaleCount( } // when a cache entry that is NOT Stale is evicted for any reason, staleness count should NOT be deducted - public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStaleCount() throws Exception { + public void testStaleCount_OnRemovalNotificationOfNonStaleKey_DoesNotDecrementsStaleCount() throws Exception { threadPool = getThreadPool(); Settings settings = Settings.builder().put(INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING.getKey(), "0.51").build(); cache = getIndicesRequestCache(settings); writer.addDocument(newDoc(0, "foo")); + ShardId shardId = indexShard.shardId(); DirectoryReader reader = getReader(writer, indexShard.shardId()); DirectoryReader secondReader = getReader(writer, indexShard.shardId()); @@ -498,6 +516,15 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStal // evict entry from second reader (this reader is not closed) IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(secondReader)); + // test the mapping + ConcurrentMap> cleanupKeyToCountMap = cache.cacheCleanupManager.getCleanupKeyToCountMap(); + // shard id should exist + assertTrue(cleanupKeyToCountMap.containsKey(shardId)); + // reader CacheKeyId should NOT exist + assertFalse(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(reader))); + // secondReader CacheKeyId should exist + assertTrue(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(secondReader))); + cache.onRemoval( new RemovalNotification, BytesReference>( new ICacheKey<>(key), @@ -505,6 +532,11 @@ public void testStaleCount_OnRemovalNotificationOfStaleKey_DoesNotDecrementsStal RemovalReason.EVICTED ) ); + + // test the mapping, shardId entry should be cleaned up + // shard id should NOT exist + assertFalse(cleanupKeyToCountMap.containsKey(shardId)); + staleKeysCount = cache.cacheCleanupManager.getStaleKeysCount(); // eviction of NON-stale key from the cache should NOT decrement staleKeysCount in iRC assertEquals(1, staleKeysCount.get()); @@ -658,27 +690,65 @@ public void testCleanupKeyToCountMapAreSetAppropriately() throws Exception { // test the mapping assertEquals(2, cache.count()); assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(secondReader))); + // create another entry for the second reader + cache.getOrCompute(getEntity(indexShard), getLoader(secondReader), secondReader, getTermBytes("id", "1")); + // test the mapping + assertEquals(3, cache.count()); + assertEquals(2, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(secondReader))); // Close the reader, to create stale entries reader.close(); - // 1 out of 2 keys ie 50% are now stale. - assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); // cache count should not be affected - assertEquals(2, cache.count()); - // test the mapping + assertEquals(3, cache.count()); + // test the mapping, first reader's entry should be removed from the mapping and accounted for in the staleKeysCount assertFalse(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(reader))); + assertEquals(1, cache.cacheCleanupManager.getStaleKeysCount().get()); // second reader's mapping should not be affected + assertEquals(2, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(secondReader))); + // send removal notification for first reader + IndicesRequestCache.Key key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(reader)); + cache.onRemoval( + new RemovalNotification, BytesReference>( + new ICacheKey<>(key), + getTermBytes(), + RemovalReason.EVICTED + ) + ); + // test the mapping, it should stay the same + assertFalse(cleanupKeyToCountMap.get(shardId).containsKey(getReaderCacheKeyId(reader))); + // staleKeysCount should be decremented + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + // second reader's mapping should not be affected + assertEquals(2, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(secondReader))); + + // Without closing the secondReader send removal notification of one of its key + key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(secondReader)); + cache.onRemoval( + new RemovalNotification, BytesReference>( + new ICacheKey<>(key), + getTermBytes(), + RemovalReason.EVICTED + ) + ); + // staleKeysCount should be the same as before + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + // secondReader's readerCacheKeyId count should be decremented by 1 assertEquals(1, (int) cleanupKeyToCountMap.get(shardId).get(getReaderCacheKeyId(secondReader))); - // Close the second reader - secondReader.close(); - // both keys should now be stale - assertEquals(2, cache.cacheCleanupManager.getStaleKeysCount().get()); - // cache count should not be affected - assertEquals(2, cache.count()); - // test the mapping - // since all the readers of this shard is closed, - // the cleanupKeyToCountMap should have no entries + // Without closing the secondReader send removal notification of its last key + key = new IndicesRequestCache.Key(indexShard.shardId(), getTermBytes(), getReaderCacheKeyId(secondReader)); + cache.onRemoval( + new RemovalNotification, BytesReference>( + new ICacheKey<>(key), + getTermBytes(), + RemovalReason.EVICTED + ) + ); + // staleKeysCount should be the same as before + assertEquals(0, cache.cacheCleanupManager.getStaleKeysCount().get()); + // since all the readers of this shard is closed, the cleanupKeyToCountMap should have no entries assertEquals(0, cleanupKeyToCountMap.size()); + + IOUtils.close(secondReader); } private DirectoryReader getReader(IndexWriter writer, ShardId shardId) throws IOException { @@ -715,6 +785,11 @@ private BytesReference getTermBytes() throws IOException { return XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); } + private BytesReference getTermBytes(String fieldName, String value) throws IOException { + TermQueryBuilder termQuery = new TermQueryBuilder(fieldName, value); + return XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + } + private String getReaderCacheKeyId(DirectoryReader reader) { OpenSearchDirectoryReader.DelegatingCacheHelper delegatingCacheHelper = (OpenSearchDirectoryReader.DelegatingCacheHelper) reader .getReaderCacheHelper();