Use caching to avoid excessive creation of NonClosingReaderWrapper instances by kkewwei · Pull Request #20921 · opensearch-project/OpenSearch

kkewwei · 2026-03-19T07:10:28Z

…stances

Description

Instead of creating a NonClosingReaderWrapper per query/update, we reuse the same NonClosingReaderWrapper by caching.

We can't reuse it in external plugin(like security plugin), in security plugin, the result may depend on conditions such as the user, so the same NonClosingReaderWrapper cannot be reused universally.

Related Issues

Resolves #19175

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-03-19T07:12:01Z

PR Reviewer Guide 🔍

(Review updated until commit `31a701b`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
📝 TODO sections server/src/main/java/org/opensearch/index/shard/IndexShard.java [2247]: Mark this as deprecated once new searcher interface is integrated into the IndexShard
🔀 No multiple PR themes
⚡ Recommended focus areas for review Cache Invalidation Race In `computeIfAbsent`, the close listener is registered with `key` but the `NonClosingReaderWrapper` is created with `directoryReader` (the outer variable). While these should be the same object, using `key` consistently inside the lambda would be safer and clearer. More importantly, if `addReaderCloseListener` throws an IOException after the entry is conceptually being created, the `remove` in the catch block may not be needed since `computeIfAbsent` would not have stored the value yet — but if it does get stored before the exception, there could be a stale entry. The exception handling inside `computeIfAbsent` should be carefully validated. nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); return new NonClosingReaderWrapper(directoryReader); } catch (IOException e) { readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); Thread Safety The `readerWrapperCache` is a `ConcurrentHashMap`, but the close listener registered via `addReaderCloseListener` calls `readerWrapperCache.remove(key)` using the outer `key` variable captured in the lambda. If the same `DirectoryReader` is closed and a new one with the same reference is added concurrently, there could be subtle issues. Additionally, the cache is cleared in `close()` outside of `computeIfAbsent` atomicity guarantees — verify that clearing the cache while `wrapSearcher` is being called concurrently does not cause issues. DirectoryReader nonClosingReaderWrapper; if (readerWrapperCache == null) { nonClosingReaderWrapper = new NonClosingReaderWrapper(directoryReader); } else { nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); return new NonClosingReaderWrapper(directoryReader); } catch (IOException e) { readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); } Cached Wrapper Reuse The cache stores the `NonClosingReaderWrapper` keyed by `DirectoryReader`, but `readerWrapper.apply(nonClosingReaderWrapper)` is called every time even when the cached `nonClosingReaderWrapper` is reused. This means a new wrapped `DirectoryReader` (the result of `readerWrapper.apply(...)`) is created on every call. The PR description says "reuse the same NonClosingReaderWrapper" — only the `NonClosingReaderWrapper` is reused, not the final wrapped reader. This should be clearly documented, and it should be verified that calling `readerWrapper.apply` repeatedly on the same `NonClosingReaderWrapper` is safe and correct. DirectoryReader reader = readerWrapper.apply(nonClosingReaderWrapper); if (reader != nonClosingReaderWrapper) { if (reader.getReaderCacheHelper() != openSearchDirectoryReader.getReaderCacheHelper()) { throw new IllegalStateException( "wrapped directory reader doesn't delegate IndexReader#getCoreCacheKey," + " wrappers must override this method and delegate to the original readers core cache key. Wrapped readers can't be " + "used as cache keys since their are used only per request which would lead to subtle bugs" ); } if (OpenSearchDirectoryReader.getOpenSearchDirectoryReader(reader) != openSearchDirectoryReader) { // prevent that somebody wraps with a non-filter reader throw new IllegalStateException("wrapped directory reader hides actual OpenSearchDirectoryReader but shouldn't"); } } Public API Change A new overload of the `public static` method `wrapSearcher` is introduced that accepts a `ConcurrentHashMap` parameter. Exposing `ConcurrentHashMap` as a concrete type in a public API (rather than `Map`) is a design concern. Additionally, this new overload is public and static, making it part of the external API surface — external plugins could call it with a shared cache in unexpected ways. public static Engine.Searcher wrapSearcher( Engine.Searcher engineSearcher, CheckedFunction<DirectoryReader, DirectoryReader, IOException> readerWrapper, ConcurrentHashMap<DirectoryReader, DirectoryReader> readerWrapperCache ) throws IOException {

kkewwei · 2026-03-19T07:12:21Z

@sgup432 Please help review in your spare time.

github-actions · 2026-03-19T07:12:30Z

PR Code Suggestions ✨

Latest suggestions up to 31a701b

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Clear cache after engine is closed Clearing the `readerWrapperCache` before closing the engine may cause issues if any in-flight searchers still hold references to cached wrappers. The cache should be cleared after the engine is closed to ensure no new searchers can be created that would use stale cache entries, and existing searchers have been released. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2378-2379] -readerWrapperCache.clear(); - final Indexer engine = this.currentEngineReference.getAndSet(null); +final Indexer engine = this.currentEngineReference.getAndSet(null); + try { + if (engine != null && flushEngine) { + engine.flushAndClose(); + } + } finally { + readerWrapperCache.clear(); + } Suggestion importance[1-10]: 5 __ Why: Clearing `readerWrapperCache` before the engine is closed could theoretically cause issues with in-flight searchers, but the `improved_code` restructures the entire close logic significantly beyond just moving the `clear()` call, making it a more invasive change. The concern is valid but the risk is moderate since the shard state is already set to CLOSED before this point.	Low
General	Use lambda parameter consistently inside lambda body Inside the `computeIfAbsent` lambda, `new NonClosingReaderWrapper(directoryReader)` should use `key` instead of the captured `directoryReader` variable. While they refer to the same object in this context, using `key` is more correct and consistent with the lambda's parameter, avoiding potential confusion or bugs if the code is refactored. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2272-2280] nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); - return new NonClosingReaderWrapper(directoryReader); + return new NonClosingReaderWrapper(key); } catch (IOException e) { readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); Suggestion importance[1-10]: 4 __ Why: Using `key` instead of the captured `directoryReader` variable inside the lambda is more consistent and avoids potential confusion during refactoring. However, since `key` and `directoryReader` refer to the same object in this context, this is a minor style improvement with no functional impact.	Low

Previous suggestions

Suggestions up to commit a72da36

Category	Suggestion	Impact
Possible issue	Fix incorrect cache key used in close listener The close listener uses `key` (the outer lambda parameter) to remove from the cache, but the listener callback parameter `cacheKey` represents the actual cache key passed by the close listener mechanism. These may differ, and using `key` instead of `cacheKey` inside the listener could cause incorrect cache entries to be removed. The listener callback should use `cacheKey` for the removal. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2272-2280] nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { - OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); + OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(cacheKey)); return new NonClosingReaderWrapper(directoryReader); } catch (IOException e) { readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); Suggestion importance[1-10]: 6 __ Why: The suggestion points out that `key` and `cacheKey` might differ in the close listener callback. However, looking at the `addReaderCloseListener` API, `cacheKey` is typically the `IndexReader.CacheKey` object, not the `DirectoryReader` itself, so using `cacheKey` for `readerWrapperCache.remove()` would actually be incorrect since the map key is `DirectoryReader`. The current code using `key` (the `DirectoryReader`) is likely correct. This suggestion may introduce a bug rather than fix one.	Low
Possible issue	Avoid swallowing checked exceptions inside lambda `computeIfAbsent` does not allow throwing checked exceptions, so the `IOException` from `addReaderCloseListener` is caught and rethrown as an unchecked `OpenSearchException`. However, if the exception is thrown inside `computeIfAbsent`, the mapping will not be inserted but the `remove` call is redundant (nothing was inserted). More critically, the unchecked exception will propagate out of `computeIfAbsent` and then out of `wrapSearcher`, bypassing the `IOException` declared in the method signature — callers expecting `IOException` won't catch it. Consider restructuring to handle the `IOException` outside of `computeIfAbsent`. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2272-2280] -nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { - try { - OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); - return new NonClosingReaderWrapper(directoryReader); - } catch (IOException e) { - readerWrapperCache.remove(key); - throw new OpenSearchException("failed to wrap searcher", e); - } - }); +nonClosingReaderWrapper = readerWrapperCache.get(directoryReader); + if (nonClosingReaderWrapper == null) { + NonClosingReaderWrapper newWrapper = new NonClosingReaderWrapper(directoryReader); + DirectoryReader existing = readerWrapperCache.putIfAbsent(directoryReader, newWrapper); + if (existing == null) { + OpenSearchDirectoryReader.addReaderCloseListener(directoryReader, cacheKey -> readerWrapperCache.remove(cacheKey)); + nonClosingReaderWrapper = newWrapper; + } else { + nonClosingReaderWrapper = existing; + } + } Suggestion importance[1-10]: 5 __ Why: The concern about wrapping `IOException` in an unchecked `OpenSearchException` inside `computeIfAbsent` is valid — callers expecting `IOException` won't catch it. The improved code restructures the logic to handle `IOException` outside the lambda, which is a cleaner approach. However, the improved code also changes the close listener to use `cacheKey` instead of `directoryReader`, which may be incorrect as noted in suggestion 1.	Low
General	Clear cache after engine shutdown to prevent leaks Clearing the `readerWrapperCache` before closing the engine means that any `NonClosingReaderWrapper` instances still referenced by active searchers won't be properly cleaned up. The cache should be cleared after the engine is closed to ensure all searchers are released first, preventing potential resource leaks or use-after-close issues. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2378-2379] -readerWrapperCache.clear(); - final Indexer engine = this.currentEngineReference.getAndSet(null); +final Indexer engine = this.currentEngineReference.getAndSet(null); + try { + if (engine != null && flushEngine) { + engine.flushAndClose(); + } + } finally { + readerWrapperCache.clear(); + } Suggestion importance[1-10]: 4 __ Why: The suggestion to clear the cache after engine shutdown has some merit for ordering, but the `NonClosingReaderWrapper` instances don't hold resources that need engine-lifecycle management — they're non-closing wrappers. The cache is already cleaned up via close listeners when readers are closed. The ordering concern is valid but low impact.	Low

Suggestions up to commit 1d4bbfa

Category	Suggestion	Impact
Possible issue	Fix inconsistent variable usage in cache lambda Inside the `computeIfAbsent` lambda, `new NonClosingReaderWrapper(directoryReader)` uses the outer variable `directoryReader` instead of the lambda parameter `key`. These should be the same object, but using `key` is more correct and consistent, especially since the close listener already uses `key`. More critically, if `addReaderCloseListener` throws an `IOException`, the entry will never have been inserted, so calling `readerWrapperCache.remove(key)` in the catch block is unnecessary and misleading — but the real issue is that the exception is thrown inside `computeIfAbsent`, which may leave the map in an inconsistent state depending on the JDK implementation. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2272-2280] nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { + NonClosingReaderWrapper wrapper = new NonClosingReaderWrapper(key); OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); - return new NonClosingReaderWrapper(directoryReader); + return wrapper; } catch (IOException e) { - readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); Suggestion importance[1-10]: 6 __ Why: The suggestion to use `key` instead of `directoryReader` inside the lambda is a valid correctness improvement for consistency. The more important fix of creating the `NonClosingReaderWrapper` before calling `addReaderCloseListener` ensures the close listener is only registered if the wrapper is successfully created, avoiding a potential state inconsistency. The removal of the unnecessary `readerWrapperCache.remove(key)` in the catch block is also correct since the entry was never inserted.	Low
General	Clear cache after engine shutdown, not before Clearing the `readerWrapperCache` before closing the engine means that if any in-flight searchers still hold references to cached `NonClosingReaderWrapper` instances, those wrappers could be garbage collected or reused incorrectly. The cache should be cleared after the engine is closed to ensure all searchers have been released first. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2378-2379] -readerWrapperCache.clear(); - final Indexer engine = this.currentEngineReference.getAndSet(null); +final Indexer engine = this.currentEngineReference.getAndSet(null); + try { + if (engine != null && flushEngine) { + engine.flushAndClose(); + } + } finally { + readerWrapperCache.clear(); + } Suggestion importance[1-10]: 5 __ Why: The concern about clearing `readerWrapperCache` before the engine is closed has merit, as in-flight searchers might still reference cached wrappers. However, `NonClosingReaderWrapper` is designed to not close the underlying reader, so clearing the cache early is less risky than suggested. The improved ordering is still a reasonable defensive practice.	Low

Suggestions up to commit 9b78f3e

Category	Suggestion	Impact
Possible issue	Clear cache after engine shutdown, not before Clearing the cache before closing the engine may cause issues if any in-flight searcher operations still reference cached wrappers. The cache should be cleared after the engine is closed to ensure no new searchers are created that could reference stale cache entries, and to avoid clearing entries that are still in use. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2378-2379] -readerWrapperCache.clear(); - final Indexer engine = this.currentEngineReference.getAndSet(null); +final Indexer engine = this.currentEngineReference.getAndSet(null); + try { + if (engine != null && flushEngine) { + engine.flushAndClose(); + } + } finally { + readerWrapperCache.clear(); + } Suggestion importance[1-10]: 5 __ Why: Clearing `readerWrapperCache` before the engine is closed could theoretically cause issues if in-flight searchers still reference cached wrappers. However, the shard is already in `CLOSED` state at this point, so new searchers shouldn't be created, making this a moderate concern rather than a critical bug.	Low
General	Use lambda parameter consistently inside lambda body Inside the `computeIfAbsent` lambda, `new NonClosingReaderWrapper(directoryReader)` should use `key` instead of the captured outer variable `directoryReader`. While they refer to the same object in this context, using `key` is more correct and consistent with the lambda's contract, avoiding potential confusion or bugs if the code is refactored. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2272-2280] nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); - return new NonClosingReaderWrapper(directoryReader); + return new NonClosingReaderWrapper(key); } catch (IOException e) { readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); Suggestion importance[1-10]: 4 __ Why: While `key` and `directoryReader` refer to the same object in this context, using `key` inside the lambda is more consistent and avoids potential confusion during refactoring. This is a minor style/correctness improvement with low risk.	Low
General	Ensure correct cache key used in close listener The close listener uses `readerWrapperCache.remove(key)` where `key` is the `DirectoryReader`, but the cache key is also `directoryReader`. If `addReaderCloseListener` fires with a `cacheKey` that differs from `key` (e.g., a sub-reader), the removal would silently fail and the cache entry would leak. The removal should use the `key` captured in the lambda rather than the `cacheKey` parameter to ensure correct eviction. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2272-2280] nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); - return new NonClosingReaderWrapper(directoryReader); + return new NonClosingReaderWrapper(key); } catch (IOException e) { readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); Suggestion importance[1-10]: 3 __ Why: The existing code already uses `readerWrapperCache.remove(key)` in the close listener (not `cacheKey`), so the suggestion's concern about using `cacheKey` is not actually present in the code. The `improved_code` only changes `new NonClosingReaderWrapper(directoryReader)` to `new NonClosingReaderWrapper(key)`, which is the same as suggestion 1 and doesn't address the stated concern about the close listener.	Low

Suggestions up to commit 5f67c69

Category	Suggestion	Impact
General	Clear cache on shard close to prevent memory leaks The `readerWrapperCache` is an instance field that is never cleared when the shard is closed. If the close listener fails to fire for any reason, or if entries accumulate, this could cause a memory leak. Consider clearing the cache in the shard's `close()` method to ensure proper cleanup. server/src/main/java/org/opensearch/index/shard/IndexShard.java [325] private final ConcurrentHashMap<DirectoryReader, NonClosingReaderWrapper> readerWrapperCache = new ConcurrentHashMap<>(); +// Ensure readerWrapperCache.clear() is called in the shard close/cleanup path Suggestion importance[1-10]: 4 __ Why: This is a valid concern about potential memory leaks if close listeners don't fire, but the 'improved_code' only adds a comment rather than actually implementing the fix. The suggestion raises a legitimate issue but doesn't provide actionable code changes.	Low
General	Verify cache eviction timing assumption in test The test asserts that `readerWrapperCache` is empty after closing `open`, but the close listener removal from the cache is asynchronous or event-driven. There is no guarantee that the cache has been updated by the time `assertTrue` is called. Consider adding a wait/poll mechanism or verifying the listener is synchronously invoked before asserting. server/src/test/java/org/opensearch/index/shard/IndexReaderWrapperTests.java [246-248] IOUtils.close(open, writer, dir); +// The close listener should fire synchronously upon reader close +assertTrue("Cache should be empty after reader is closed", readerWrapperCache.isEmpty()); - assertTrue(readerWrapperCache.isEmpty()); - Suggestion importance[1-10]: 2 __ Why: The 'improved_code' only adds a message to the `assertTrue` call without addressing the actual timing concern. The suggestion raises a valid question about synchronous vs asynchronous listener invocation, but the improvement is minimal and the `existing_code` vs `improved_code` are nearly identical.	Low
Possible issue	Use lambda parameter consistently in cache operations The close listener uses `key` (the `directoryReader`) to remove from the cache, but the listener callback receives a `cacheKey` parameter which is the actual key used by the reader's cache helper. These may differ, causing the entry to never be removed from the cache. The listener should use `cacheKey` to remove the entry, or ensure the correct key is used. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2272-2280] nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { OpenSearchDirectoryReader.addReaderCloseListener(key, cacheKey -> readerWrapperCache.remove(key)); - return new NonClosingReaderWrapper(directoryReader); + return new NonClosingReaderWrapper(key); } catch (IOException e) { readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); Suggestion importance[1-10]: 3 __ Why: The suggestion claims the close listener uses the wrong key, but looking at the code, `readerWrapperCache.remove(key)` inside the lambda correctly captures `key` (the `directoryReader`), which is the same key used in `computeIfAbsent`. The `cacheKey` in the listener callback is a different concept (the reader's cache helper key), but the map key is `directoryReader`/`key`. The 'improved_code' only changes `directoryReader` to `key` in `new NonClosingReaderWrapper(key)`, which is a minor consistency improvement but not a bug fix as claimed.	Low

Suggestions up to commit 1f8febd

Category	Suggestion	Impact
General	Use lambda parameter consistently inside computeIfAbsent Inside the `computeIfAbsent` lambda, `new NonClosingReaderWrapper(directoryReader)` should use `key` instead of the captured `directoryReader` variable. While they refer to the same object in this context, using `key` is more correct and consistent with the close listener which already uses `key`. More critically, if the close listener fires and removes `key` from the cache, but then an exception is thrown after the listener is added but before the wrapper is returned, the `readerWrapperCache.remove(key)` in the catch block is redundant but harmless — however, the listener registration should happen after the wrapper is successfully created to avoid registering a listener for a wrapper that was never cached. server/src/main/java/org/opensearch/index/shard/IndexShard.java [2272-2283] nonClosingReaderWrapper = readerWrapperCache.computeIfAbsent(directoryReader, key -> { try { + NonClosingReaderWrapper wrapper = new NonClosingReaderWrapper(key); OpenSearchDirectoryReader.addReaderCloseListener( key, cacheKey -> readerWrapperCache.remove(key) ); - return new NonClosingReaderWrapper(directoryReader); + return wrapper; } catch (IOException e) { readerWrapperCache.remove(key); throw new OpenSearchException("failed to wrap searcher", e); } }); Suggestion importance[1-10]: 6 __ Why: The suggestion to create the `NonClosingReaderWrapper` before registering the close listener is a valid ordering improvement — it avoids registering a listener for a wrapper that was never successfully cached. Using `key` instead of the captured `directoryReader` is also more consistent, though functionally equivalent here.	Low
	Prevent potential memory leak in cache The `readerWrapperCache` is an instance field that grows as new `DirectoryReader` instances are added, and entries are only removed via a close listener. If the close listener is never triggered (e.g., due to a bug or an unclosed reader), the cache will leak memory. Consider using a `WeakHashMap`-based approach or adding a bounded size to prevent unbounded growth, or at minimum ensure the cache is cleared when the shard is closed. server/src/main/java/org/opensearch/index/shard/IndexShard.java [325] private final ConcurrentHashMap<DirectoryReader, NonClosingReaderWrapper> readerWrapperCache = new ConcurrentHashMap<>(); +// Note: ensure readerWrapperCache.clear() is called in the shard close/cleanup path to prevent memory leaks if close listeners are not reliably fired. Suggestion importance[1-10]: 2 __ Why: The suggestion raises a valid concern about potential memory leaks, but the `improved_code` is identical to the `existing_code` with only a comment added, which doesn't actually fix the issue. The suggestion score is low as it only adds a comment without implementing a real solution.	Low
	Improve test assertion reliability after close The test asserts that `readerWrapperCache.isEmpty()` after closing `open`, relying on the close listener being invoked synchronously during `IOUtils.close`. However, if the close listener is asynchronous or not guaranteed to fire immediately, this assertion could be flaky. It would be safer to verify the cache removal more explicitly or add a comment explaining the synchronous guarantee. server/src/test/java/org/opensearch/index/shard/IndexReaderWrapperTests.java [246-248] IOUtils.close(open, writer, dir); +// The close listener registered via addReaderCloseListener should have fired synchronously on close, +// removing the entry from the cache. +assertTrue("Cache should be empty after reader is closed", readerWrapperCache.isEmpty()); - assertTrue(readerWrapperCache.isEmpty()); - Suggestion importance[1-10]: 2 __ Why: The `improved_code` only adds a comment and a message to the assertion without actually changing the test logic, making it functionally equivalent to the `existing_code`. The concern about asynchronous close listeners is valid but the fix is minimal.	Low

github-actions · 2026-03-19T07:45:11Z

Persistent review updated to latest commit 1f8febd

github-actions · 2026-03-19T07:57:24Z

❌ Gradle check result for 1f8febd: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-19T07:59:31Z

Persistent review updated to latest commit 5f67c69

github-actions · 2026-03-19T09:11:52Z

❌ Gradle check result for 5f67c69: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-19T11:50:25Z

❌ Gradle check result for 5f67c69: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-19T14:14:09Z

❌ Gradle check result for 5f67c69: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

kkewwei · 2026-03-19T15:15:59Z

❌ Gradle check result for 5f67c69: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

org.opensearch.indices.IndicesRequestCacheIT.testCanCache #20920

sgup432 · 2026-03-19T20:38:23Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

     * @opensearch.internal
     */
-    private static final class NonClosingReaderWrapper extends FilterDirectoryReader {
+    @PublicApi(since = "3.6.0")


Why do we need to make this public?

IndexReaderWrapperTests needs to reference NonClosingReaderWrapper, which requires the class to be exposed. However, I believe we can avoid this issue by using DirectoryReader externally instead.

sgup432 · 2026-03-19T20:41:55Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

-        NonClosingReaderWrapper nonClosingReaderWrapper = new NonClosingReaderWrapper(engineSearcher.getDirectoryReader());
+
+        NonClosingReaderWrapper nonClosingReaderWrapper;
+        if (readerWrapperCache == null) {


I guess this would never be null?

Being a static function, wrapSearcher is invoked directly within InternalEngine. Since InternalEngine does not have access to the variables in the corresponding IndexShard, these variables will be set to null upon entry into this function.

sgup432 · 2026-03-19T20:50:08Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java


    private final IndexingOperationListener indexingOperationListeners;
    private final Runnable globalCheckpointSyncer;
+    private final ConcurrentHashMap<DirectoryReader, NonClosingReaderWrapper> readerWrapperCache = new ConcurrentHashMap<>();


Lets clear this map during IndexShard.close() as well? Though it would eventually clear it out when all underlying segments are closed, but still good to have?

github-actions · 2026-03-20T02:12:20Z

Persistent review updated to latest commit 9b78f3e

github-actions · 2026-03-20T03:05:06Z

Persistent review updated to latest commit 1d4bbfa

github-actions · 2026-03-20T03:09:38Z

Persistent review updated to latest commit a72da36

…stances Signed-off-by: kkewwei <kewei.11@bytedance.com> Signed-off-by: kkewwei <kkewwei@163.com>

Signed-off-by: kkewwei <kewei.11@bytedance.com> Signed-off-by: kkewwei <kkewwei@163.com>

github-actions · 2026-03-20T04:00:13Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit e7344dd.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/search/aggregations/bucket/terms/SignificantTextAggregatorFactory.java	318	medium	Thread-safety guard removed from supportsConcurrentSegmentSearch(). The original code explicitly returned 'filterDuplicateText == false' to prevent concurrent access to the non-thread-safe DuplicateByteSequenceSpotter, with a comment explaining this was intentional. The change unconditionally returns true, potentially enabling concurrent segment search when filterDuplicateText is active. The CHANGELOG entry 'Disable concurrent search for filter duplicates in significant_text (#20857)' was simultaneously removed from the diff, suggesting the prior safety fix is being reversed. No corresponding fix to make DuplicateByteSequenceSpotter thread-safe is visible in this diff, warranting investigation into whether the concurrency hazard was resolved elsewhere.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

Signed-off-by: kkewwei <kewei.11@bytedance.com> Signed-off-by: kkewwei <kkewwei@163.com>

github-actions · 2026-03-20T04:04:08Z

Persistent review updated to latest commit 31a701b

github-actions · 2026-03-20T05:10:13Z

❌ Gradle check result for 31a701b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-20T07:14:36Z

✅ Gradle check result for 31a701b: SUCCESS

kkewwei requested review from a team, Bukhtawar, CEHENKLE, Rishikesh1159, anasalkouz, andrross, ashking94, cwperks, dbwiddis, gbbafna, jed326, kotwanikunal, mch2, msfroh, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners March 19, 2026 07:10

github-actions bot added enhancement Enhancement or improvement to existing feature or request lucene Search:Performance labels Mar 19, 2026

opensearch-ci-bot mentioned this pull request Mar 19, 2026

[AUTOCUT] Gradle Check Flaky Test Report for IndicesRequestCacheIT #14288

Open

sgup432 reviewed Mar 19, 2026

View reviewed changes

kkewwei added 3 commits March 20, 2026 11:57

Use caching to avoid excessive creation of NonClosingReaderWrapper in…

ce76483

…stances Signed-off-by: kkewwei <kewei.11@bytedance.com> Signed-off-by: kkewwei <kkewwei@163.com>

fix by github-actions

ab39b77

Signed-off-by: kkewwei <kewei.11@bytedance.com> Signed-off-by: kkewwei <kkewwei@163.com>

fix spotlessApply

e844651

Signed-off-by: kkewwei <kewei.11@bytedance.com> Signed-off-by: kkewwei <kkewwei@163.com>

kkewwei force-pushed the reuse_same_Reade branch from a72da36 to e7344dd Compare March 20, 2026 03:58

kkewwei added 2 commits March 20, 2026 12:00

little fix

fd7053b

Signed-off-by: kkewwei <kewei.11@bytedance.com> Signed-off-by: kkewwei <kkewwei@163.com>

Merge branch 'main' into HEAD

31a701b

kkewwei force-pushed the reuse_same_Reade branch from e7344dd to 31a701b Compare March 20, 2026 04:02

opensearch-ci-bot mentioned this pull request Mar 20, 2026

[AUTOCUT] Gradle Check Flaky Test Report for RecoveryWhileUnderLoadIT #14509

Open

Conversation

kkewwei commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

github-actions bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 31a701b)

Uh oh!

kkewwei commented Mar 19, 2026

Uh oh!

github-actions bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

kkewwei commented Mar 19, 2026

Uh oh!

sgup432 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

kkewwei Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

sgup432 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

kkewwei Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

sgup432 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 20, 2026

Uh oh!

github-actions bot commented Mar 20, 2026

Uh oh!

github-actions bot commented Mar 20, 2026

Uh oh!

github-actions bot commented Mar 20, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 20, 2026

Uh oh!

github-actions bot commented Mar 20, 2026

Uh oh!

github-actions bot commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kkewwei commented Mar 19, 2026 •

edited

Loading

github-actions bot commented Mar 19, 2026 •

edited

Loading

(Review updated until commit `31a701b`)

github-actions bot commented Mar 19, 2026 •

edited

Loading