From a8ad16409f4da553cd9856e4352ab544d59513ec Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Tue, 11 Jun 2024 21:03:56 -0700 Subject: [PATCH 1/3] Fix for ShardNotFoundException during request cache clean up Signed-off-by: Sagar Upadhyaya --- .../indices/IndicesRequestCache.java | 3 +- .../opensearch/indices/IndicesService.java | 2 +- .../indices/IndicesRequestCacheTests.java | 68 ++++++++++++++++--- 3 files changed, 62 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 57f7e402536f2..5c82e5e9639f7 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java @@ -153,7 +153,8 @@ public final class IndicesRequestCache implements RemovalListener cache; private final ClusterService clusterService; - private final Function> cacheEntityLookup; + // pkg-private for testing + final Function> cacheEntityLookup; // pkg-private for testing final IndicesRequestCacheCleanupManager cacheCleanupManager; diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 251be8a990055..73b4cf3259d5b 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -404,7 +404,7 @@ public IndicesService( if (indexService == null) { return Optional.empty(); } - return Optional.of(new IndexShardCacheEntity(indexService.getShard(shardId.id()))); + return Optional.of(new IndexShardCacheEntity(indexService.getShardOrNull(shardId.id()))); }), cacheService, threadPool, clusterService); this.indicesQueryCache = new IndicesQueryCache(settings); this.mapperRegistry = mapperRegistry; diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java index dcddd9f3d1318..80c4340609b05 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -798,15 +798,9 @@ private DirectoryReader getReader(IndexWriter writer, ShardId shardId) throws IO private IndicesRequestCache getIndicesRequestCache(Settings settings) { 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()))); - }), + return new IndicesRequestCache( + settings, + indicesService.indicesRequestCache.cacheEntityLookup, new CacheModule(new ArrayList<>(), Settings.EMPTY).getCacheService(), threadPool, ClusterServiceUtils.createClusterService(threadPool) @@ -1419,6 +1413,62 @@ public void testDeleteAndCreateIndexShardOnSameNodeAndVerifyStats() throws Excep IOUtils.close(reader, writer, dir, cache); } + public void testIndexShardClosedAndVerifyCacheCleanUpWorksSuccessfully() throws Exception { + threadPool = getThreadPool(); + String indexName = "test1"; + IndicesService indicesService = getInstanceFromNode(IndicesService.class); + // Create a shard + IndexService indexService = createIndex( + indexName, + Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0).build() + ); + Index idx = resolveIndex(indexName); + ShardRouting shardRouting = indicesService.indexService(idx).getShard(0).routingEntry(); + IndexShard indexShard = indexService.getShard(0); + Directory dir = newDirectory(); + IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); + writer.addDocument(newDoc(0, "foo")); + writer.addDocument(newDoc(1, "hack")); + DirectoryReader reader = OpenSearchDirectoryReader.wrap(DirectoryReader.open(writer), indexShard.shardId()); + Loader loader = new Loader(reader, 0); + + // Set clean interval to a high value as we will do it manually here. + IndicesRequestCache cache = getIndicesRequestCache( + Settings.builder() + .put(IndicesRequestCache.INDICES_REQUEST_CACHE_CLEANUP_INTERVAL_SETTING_KEY, TimeValue.timeValueMillis(100000)) + .build() + ); + IndicesService.IndexShardCacheEntity cacheEntity = new IndicesService.IndexShardCacheEntity(indexShard); + TermQueryBuilder termQuery = new TermQueryBuilder("id", "bar"); + BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); + + // Cache some values for indexShard + BytesReference value = cache.getOrCompute(cacheEntity, loader, reader, getTermBytes()); + + // Verify response and stats. + assertEquals("foo", value.streamInput().readString()); + RequestCacheStats stats = indexShard.requestCache().stats(); + assertEquals("foo", value.streamInput().readString()); + assertEquals(1, cache.count()); + assertEquals(1, stats.getMissCount()); + assertTrue(stats.getMemorySizeInBytes() > 0); + System.out.println("memory = " + stats.getMemorySizeInBytes()); + + // Remove the shard making its cache entries stale + IOUtils.close(reader, writer, dir); + indexService.removeShard(0, "force"); + System.out.println("index stats = " + indexShard.state()); + + assertBusy(() -> { assertEquals(IndexShardState.CLOSED, indexShard.state()); }); + + // Trigger clean up of cache. Should not throw any exception. + cache.cacheCleanupManager.cleanCache(); + // Verify all cleared up. + assertEquals(0, cache.count()); + + IOUtils.close(cache); + } + public static String generateString(int length) { String characters = "abcdefghijklmnopqrstuvwxyz"; StringBuilder sb = new StringBuilder(length); From 0dca13ab8a2e7ede698fe2c097b3abb4c756614b Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Tue, 11 Jun 2024 21:10:05 -0700 Subject: [PATCH 2/3] Added changelog Signed-off-by: Sagar Upadhyaya --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a7e8e09a392d..89d84cfdb9597 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix NPE on restore searchable snapshot ([#13911](https://github.com/opensearch-project/OpenSearch/pull/13911)) - Fix double invocation of postCollection when MultiBucketCollector is present ([#14015](https://github.com/opensearch-project/OpenSearch/pull/14015)) - Java high-level REST client bulk() is not respecting the bulkRequest.requireAlias(true) method call ([#14146](https://github.com/opensearch-project/OpenSearch/pull/14146)) +- Fix ShardNotFoundException during request cache clean up ([#14219](https://github.com/opensearch-project/OpenSearch/pull/14219)) ### Security From 5d19f76847c643b6a0f98c5be7c1a3089ca47e24 Mon Sep 17 00:00:00 2001 From: Sagar Upadhyaya Date: Tue, 11 Jun 2024 21:31:20 -0700 Subject: [PATCH 3/3] Fix forbidden gradle check Signed-off-by: Sagar Upadhyaya --- .../opensearch/indices/IndicesRequestCacheTests.java | 10 ++-------- 1 file changed, 2 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 80c4340609b05..9dbdddb76ea24 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java @@ -105,6 +105,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import static java.util.Collections.emptyMap; @@ -1416,14 +1417,11 @@ public void testDeleteAndCreateIndexShardOnSameNodeAndVerifyStats() throws Excep public void testIndexShardClosedAndVerifyCacheCleanUpWorksSuccessfully() throws Exception { threadPool = getThreadPool(); String indexName = "test1"; - IndicesService indicesService = getInstanceFromNode(IndicesService.class); // Create a shard IndexService indexService = createIndex( indexName, Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0).build() ); - Index idx = resolveIndex(indexName); - ShardRouting shardRouting = indicesService.indexService(idx).getShard(0).routingEntry(); IndexShard indexShard = indexService.getShard(0); Directory dir = newDirectory(); IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()); @@ -1440,7 +1438,6 @@ public void testIndexShardClosedAndVerifyCacheCleanUpWorksSuccessfully() throws ); IndicesService.IndexShardCacheEntity cacheEntity = new IndicesService.IndexShardCacheEntity(indexShard); TermQueryBuilder termQuery = new TermQueryBuilder("id", "bar"); - BytesReference termBytes = XContentHelper.toXContent(termQuery, MediaTypeRegistry.JSON, false); // Cache some values for indexShard BytesReference value = cache.getOrCompute(cacheEntity, loader, reader, getTermBytes()); @@ -1452,20 +1449,17 @@ public void testIndexShardClosedAndVerifyCacheCleanUpWorksSuccessfully() throws assertEquals(1, cache.count()); assertEquals(1, stats.getMissCount()); assertTrue(stats.getMemorySizeInBytes() > 0); - System.out.println("memory = " + stats.getMemorySizeInBytes()); // Remove the shard making its cache entries stale IOUtils.close(reader, writer, dir); indexService.removeShard(0, "force"); - System.out.println("index stats = " + indexShard.state()); - assertBusy(() -> { assertEquals(IndexShardState.CLOSED, indexShard.state()); }); + assertBusy(() -> { assertEquals(IndexShardState.CLOSED, indexShard.state()); }, 1, TimeUnit.SECONDS); // Trigger clean up of cache. Should not throw any exception. cache.cacheCleanupManager.cleanCache(); // Verify all cleared up. assertEquals(0, cache.count()); - IOUtils.close(cache); }