Add module wiring and integration tests for WritableWarm tiered storage

[MayankHarsh03]

Description

Wires the remaining tiered storage components into the server and adds integration tests for warm index operations.

Module wiring:

• Node.java: Registered TieredDirectoryFactory as a composite directory factory, initialized TieredStoragePrefetchSettings from cluster settings, and bound TierActionMetrics in Guice for migration metrics tracking
• IndicesService.java: Added TieredStorageSearchSlowLog and StoredFieldsPrefetch as search operation listeners for each index, with prefetch settings supplier initialized from cluster settings
• ThreadPool.java: Added remote_download scaling thread pool executor used by BlockTransferManager for async block downloads from remote storage

All wiring is gated behind FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG.

Integration tests (WarmIndexBasicIT):

• testWritableWarm: Creates a warm index, ingests docs, searches, force merges, and verifies file cleanup from directory and file cache
• testLocalDirectoryFilesAfterRefresh: Verifies that local directory only contains block files after refresh (no full files)
• testCloseIndex: Verifies data is preserved after close and reopen of a warm index
• testWritableWarmPrimaryReplicaBoth: Verifies warm index operations with primary and replica shards

Depends on #21332

Related Issues

Resolves #21101

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]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
sandbox/plugins/analytics-backend-datafusion/build.gradle	49	high	New dependency added: 'io.substrait:isthmus:0.67.0'. Per mandatory rule, all new dependency additions must be flagged regardless of apparent legitimacy. Artifact authenticity cannot be verified without manual review.
sandbox/plugins/analytics-backend-datafusion/build.gradle	50	high	New dependency added: 'io.substrait:core:0.67.0'. Per mandatory rule, all new dependency additions must be flagged regardless of apparent legitimacy. Artifact authenticity cannot be verified without manual review.
sandbox/plugins/analytics-backend-datafusion/build.gradle	51	high	New dependency added: 'com.fasterxml.jackson.datatype:jackson-datatype-jdk8'. Per mandatory rule, all new dependency additions must be flagged for maintainer verification.
sandbox/plugins/parquet-data-format/build.gradle	28	high	Dependency version bump: 'org.checkerframework:checker-qual' changed from 3.42.0 to 3.43.0. Per mandatory rule, dependency version changes must be flagged for maintainer verification.
sandbox/plugins/analytics-backend-datafusion/licenses/core-LICENSE.txt	1	medium	The LICENSE files for 'core-0.67.0.jar' and 'isthmus-0.67.0.jar' both contain text claiming to be the 'Jackson JSON processor streaming parser/generator' license, but these JARs are io.substrait libraries (not Jackson). This license mismatch misrepresents the actual software being included and warrants investigation to confirm the correct licenses are applied and the correct artifacts are being pulled.

The table above displays the top 10 most important findings.

Total: 5 | Critical: 0 | High: 4 | 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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b4ad170)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add remote_download thread pool for tiered storage Relevant files: server/src/main/java/org/opensearch/threadpool/ThreadPool.java server/src/test/java/org/opensearch/threadpool/ScalingThreadPoolTests.java Sub-PR theme: Refactor DirectoryUtils to support FilterDirectory unwrapping Relevant files: server/src/main/java/org/opensearch/storage/utils/DirectoryUtils.java server/src/main/java/org/opensearch/storage/directory/TieredDirectory.java server/src/main/java/org/opensearch/storage/directory/TieredDirectoryFactory.java server/src/test/java/org/opensearch/storage/utils/DirectoryUtilsTests.java Sub-PR theme: Wire tiered storage components and add integration tests Relevant files: server/src/internalClusterTest/java/org/opensearch/storage/WarmIndexBasicIT.java server/src/main/java/org/opensearch/node/Node.java server/src/main/java/org/opensearch/indices/IndicesService.java server/src/test/java/org/opensearch/node/NodeTests.java

⚡ Recommended focus areas for review Duplicate Initialization TieredStoragePrefetchSettings is instantiated twice: once in Node.java (lines ~908-916) and again independently in IndicesService.java (lines ~516-521). Both create separate instances from the same cluster settings, which means settings listeners are registered twice and the two instances may diverge. A single shared instance should be passed to IndicesService. final TieredStoragePrefetchSettings tieredStoragePrefetchSettings; final Supplier<TieredStoragePrefetchSettings> tieredStoragePrefetchSettingsSupplier; if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)) { tieredStoragePrefetchSettings = new TieredStoragePrefetchSettings(clusterService.getClusterSettings()); tieredStoragePrefetchSettingsSupplier = () -> tieredStoragePrefetchSettings; } else { tieredStoragePrefetchSettings = null; tieredStoragePrefetchSettingsSupplier = () -> null; } Warm Node Assumption internalTestCluster.getDataNodeInstance(...) is used to retrieve the shard and file cache, but in tests with warm nodes (startDataAndWarmNodes), the primary shard may be placed on either a data or warm node. If the shard is on a warm node, getDataNodeInstance may return a different node's service, causing a null shard lookup or incorrect file cache reference. FileCache fileCache = internalTestCluster.getDataNodeInstance(Node.class).fileCache(); IndexShard shard = internalTestCluster.getDataNodeInstance(IndicesService.class) .indexService(resolveIndex(INDEX_NAME)) .getShardOrNull(0); Directory directory = unwrapToCompositeDirectory(shard.store().directory()); Race Condition In testLocalDirectoryFilesAfterRefresh, after waitUntil confirms block files exist, the subsequent assertTrue assertion on listLocalFiles() is not guarded — the directory state could change between the two calls, leading to a flaky test. waitUntil(() -> { try { return Arrays.stream(tieredDirectory.listLocalFiles()).anyMatch(file -> file.contains("block")); } catch (IOException ignored) { return false; } }, 30, TimeUnit.SECONDS); assertTrue( Arrays.stream(tieredDirectory.listLocalFiles()) .filter(file -> !file.contains("block")) .filter(file -> !file.contains("write.lock")) .findAny() .isEmpty() ); Error Message In unwrapFSDirectory, the exception message uses directory.getClass().getName() (the original input), but by the time the exception is thrown, current has been fully unwrapped. If the input was a FilterDirectory wrapping a non-FSDirectory, the message will show the outer wrapper class rather than the actual innermost class, which may be misleading during debugging. public static FSDirectory unwrapFSDirectory(Directory directory) { Directory current = directory; while (current instanceof FilterDirectory) { current = ((FilterDirectory) current).getDelegate(); } if (current instanceof FSDirectory) { return (FSDirectory) current; } throw new IllegalArgumentException("Expected FSDirectory but got: " + directory.getClass().getName()); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b4ad170

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid duplicate settings instances across components IndicesService creates its own TieredStoragePrefetchSettings instance, while Node.java also creates a separate instance. This results in two independent instances with potentially divergent state. The TieredStoragePrefetchSettings instance created in Node.java should be passed into IndicesService via its constructor to share a single instance. server/src/main/java/org/opensearch/indices/IndicesService.java [516-521] -if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)) { - final TieredStoragePrefetchSettings prefetchSettings = new TieredStoragePrefetchSettings(clusterService.getClusterSettings()); - this.tieredStoragePrefetchSettingsSupplier = () -> prefetchSettings; -} else { - this.tieredStoragePrefetchSettingsSupplier = () -> null; -} +// Accept tieredStoragePrefetchSettingsSupplier as a constructor parameter instead of creating a new instance here +this.tieredStoragePrefetchSettingsSupplier = tieredStoragePrefetchSettingsSupplier; Suggestion importance[1-10]: 7 __ Why: IndicesService creates its own TieredStoragePrefetchSettings instance independently from Node.java, leading to two separate instances that could diverge. Sharing a single instance via constructor injection would be cleaner and more correct, though the practical impact depends on whether settings are mutable after construction.	Medium
General	Fix misleading exception message on unwrap failure The error message in the IllegalArgumentException reports the class name of the original directory argument, but after unwrapping, the actual non-FSDirectory type found is current. The message should reference current.getClass().getName() to accurately describe what was found at the end of the chain. server/src/main/java/org/opensearch/storage/utils/DirectoryUtils.java [57-66] public static FSDirectory unwrapFSDirectory(Directory directory) { Directory current = directory; while (current instanceof FilterDirectory) { current = ((FilterDirectory) current).getDelegate(); } if (current instanceof FSDirectory) { return (FSDirectory) current; } - throw new IllegalArgumentException("Expected FSDirectory but got: " + directory.getClass().getName()); + throw new IllegalArgumentException("Expected FSDirectory but got: " + current.getClass().getName()); } Suggestion importance[1-10]: 5 __ Why: The error message uses directory.getClass().getName() (the original input) instead of current.getClass().getName() (the actual unwrapped type), which could be misleading when the input is a FilterDirectory wrapping a non-FSDirectory. The fix accurately reports what was found at the end of the chain.	Low
	Guard against null shard reference in tests getShardOrNull(0) can return null if the shard is not yet assigned or initialized on the data node, which would cause a NullPointerException when calling shard.store().directory(). Add a non-null assertion or use getShard(0) (which throws a meaningful exception) to fail fast with a clear message. server/src/internalClusterTest/java/org/opensearch/storage/WarmIndexBasicIT.java [112-116] FileCache fileCache = internalTestCluster.getDataNodeInstance(Node.class).fileCache(); IndexShard shard = internalTestCluster.getDataNodeInstance(IndicesService.class) .indexService(resolveIndex(INDEX_NAME)) - .getShardOrNull(0); + .getShard(0); Directory directory = unwrapToCompositeDirectory(shard.store().directory()); Suggestion importance[1-10]: 4 __ Why: getShardOrNull(0) can return null if the shard isn't assigned yet, causing a NullPointerException on shard.store().directory(). Using getShard(0) or adding a null check would produce a clearer failure message, though in practice ensureGreen() is called after this block rather than before, so the risk is real.	Low

---

Previous suggestions

Suggestions up to commit b4ad170

Category	Suggestion	Impact
Possible issue	Avoid duplicate settings instances across components IndicesService creates its own TieredStoragePrefetchSettings instance, while Node.java also creates a separate instance. This results in two independent instances with potentially divergent state. The TieredStoragePrefetchSettings instance created in Node.java should be passed into IndicesService via its constructor to ensure a single shared instance. server/src/main/java/org/opensearch/indices/IndicesService.java [516-521] -if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)) { - final TieredStoragePrefetchSettings prefetchSettings = new TieredStoragePrefetchSettings(clusterService.getClusterSettings()); - this.tieredStoragePrefetchSettingsSupplier = () -> prefetchSettings; -} else { - this.tieredStoragePrefetchSettingsSupplier = () -> null; -} +// Accept tieredStoragePrefetchSettingsSupplier as a constructor parameter instead of creating a new instance here +this.tieredStoragePrefetchSettingsSupplier = tieredStoragePrefetchSettingsSupplier; Suggestion importance[1-10]: 7 __ Why: IndicesService creates its own TieredStoragePrefetchSettings instance independently from Node.java, resulting in two separate instances that could diverge. Sharing a single instance via constructor injection would be cleaner and more correct, though the practical impact depends on whether settings are mutable after creation.	Medium
General	Fix misleading exception message on unwrap failure The error message in the IllegalArgumentException reports the class name of the original directory argument, but after unwrapping, current may be a different (non-FSDirectory) type. The message should report current.getClass().getName() to accurately identify the actual unwrapped type that failed the check. server/src/main/java/org/opensearch/storage/utils/DirectoryUtils.java [57-66] public static FSDirectory unwrapFSDirectory(Directory directory) { Directory current = directory; while (current instanceof FilterDirectory) { current = ((FilterDirectory) current).getDelegate(); } if (current instanceof FSDirectory) { return (FSDirectory) current; } - throw new IllegalArgumentException("Expected FSDirectory but got: " + directory.getClass().getName()); + throw new IllegalArgumentException("Expected FSDirectory but got: " + current.getClass().getName()); } Suggestion importance[1-10]: 5 __ Why: The error message reports directory.getClass().getName() (the original wrapped type) instead of current.getClass().getName() (the actual unwrapped type that failed the check), which would be more informative for debugging. This is a minor but accurate improvement to error reporting.	Low
	Fix assertion argument order in test The arguments to assertEquals are in the wrong order. JUnit/OpenSearch convention is assertEquals(expected, actual). Here docCount is the expected value and docCountUpdated is the actual value after reopen, so the arguments should be swapped to produce a meaningful failure message. server/src/internalClusterTest/java/org/opensearch/storage/WarmIndexBasicIT.java [276] -assertEquals(docCountUpdated, docCount); +assertEquals(docCount, docCountUpdated); Suggestion importance[1-10]: 5 __ Why: The assertEquals(docCountUpdated, docCount) call has arguments in the wrong order per JUnit convention (expected first, actual second). Swapping to assertEquals(docCount, docCountUpdated) produces a more meaningful failure message, though the test correctness is unaffected.	Low

Suggestions up to commit 9af0520

Category	Suggestion	Impact
Possible issue	Avoid duplicate settings instance creation IndicesService creates its own TieredStoragePrefetchSettings instance independently from the one created in Node.java, resulting in two separate instances registered against the same ClusterSettings. This can cause duplicate settings update consumers and inconsistent state. The TieredStoragePrefetchSettings instance (or its supplier) should be injected into IndicesService rather than constructed internally. server/src/main/java/org/opensearch/indices/IndicesService.java [516-521] -if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)) { - final TieredStoragePrefetchSettings prefetchSettings = new TieredStoragePrefetchSettings(clusterService.getClusterSettings()); - this.tieredStoragePrefetchSettingsSupplier = () -> prefetchSettings; -} else { - this.tieredStoragePrefetchSettingsSupplier = () -> null; -} +// Accept tieredStoragePrefetchSettingsSupplier as a constructor parameter instead of creating a new instance here +this.tieredStoragePrefetchSettingsSupplier = tieredStoragePrefetchSettingsSupplier; Suggestion importance[1-10]: 7 __ Why: Creating a second TieredStoragePrefetchSettings instance in IndicesService independently from the one in Node.java could register duplicate settings update consumers against the same ClusterSettings, leading to inconsistent state. Injecting the supplier as a constructor parameter would be the correct fix, though the actual impact depends on whether duplicate registration causes real issues.	Medium
	Ensure file cache and shard from same node getDataNodeInstance is called twice with different types, but both calls may not be guaranteed to return instances from the same data node in a multi-node cluster. This could lead to a fileCache from one node being used to assert against a directory from a different node. Both instances should be retrieved from the same node explicitly. server/src/internalClusterTest/java/org/opensearch/storage/WarmIndexBasicIT.java [112-116] -FileCache fileCache = internalTestCluster.getDataNodeInstance(Node.class).fileCache(); -IndexShard shard = internalTestCluster.getDataNodeInstance(IndicesService.class) +Node dataNode = internalTestCluster.getInstance(Node.class, internalTestCluster.getRandomNodeName()); +FileCache fileCache = dataNode.fileCache(); +IndexShard shard = dataNode.injector().getInstance(IndicesService.class) .indexService(resolveIndex(INDEX_NAME)) .getShardOrNull(0); Directory directory = unwrapToCompositeDirectory(shard.store().directory()); Suggestion importance[1-10]: 5 __ Why: In a single data+warm node setup (as started by startDataAndWarmNodes(1)), both getDataNodeInstance calls would return the same node, so the risk is low in this specific test. However, the concern is valid in principle, and the improved code would make the test more robust. The suggested improved_code uses a slightly different API pattern that may not exactly match the available test infrastructure.	Low
General	Fix misleading exception message on unwrap failure The error message in the IllegalArgumentException reports the class name of the original directory argument, but after unwrapping through multiple FilterDirectory layers, the actual non-FSDirectory type found at the end of the chain may differ. The message should report the class of current (the actual unwrapped directory) to aid debugging. server/src/main/java/org/opensearch/storage/utils/DirectoryUtils.java [57-66] public static FSDirectory unwrapFSDirectory(Directory directory) { Directory current = directory; while (current instanceof FilterDirectory) { current = ((FilterDirectory) current).getDelegate(); } if (current instanceof FSDirectory) { return (FSDirectory) current; } - throw new IllegalArgumentException("Expected FSDirectory but got: " + directory.getClass().getName()); + throw new IllegalArgumentException("Expected FSDirectory but got: " + current.getClass().getName()); } Suggestion importance[1-10]: 4 __ Why: The error message correctly identifies the issue but reports directory.getClass().getName() (the original wrapped directory) instead of current.getClass().getName() (the actual unwrapped type), which would be more useful for debugging. This is a minor but valid improvement to error reporting.	Low

Suggestions up to commit f1f484d

Category	Suggestion	Impact
Possible issue	Avoid duplicate settings instance creation IndicesService creates its own TieredStoragePrefetchSettings instance, while Node.java also creates a separate instance. These two instances will independently register settings update consumers with ClusterSettings, leading to duplicate registrations and potentially inconsistent state. The TieredStoragePrefetchSettings instance (or its supplier) should be passed into IndicesService via its constructor rather than being created internally. server/src/main/java/org/opensearch/indices/IndicesService.java [516-521] -if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)) { - final TieredStoragePrefetchSettings prefetchSettings = new TieredStoragePrefetchSettings(clusterService.getClusterSettings()); - this.tieredStoragePrefetchSettingsSupplier = () -> prefetchSettings; -} else { - this.tieredStoragePrefetchSettingsSupplier = () -> null; -} +// Accept tieredStoragePrefetchSettingsSupplier as a constructor parameter instead of creating a new instance here +this.tieredStoragePrefetchSettingsSupplier = tieredStoragePrefetchSettingsSupplier; Suggestion importance[1-10]: 7 __ Why: Both IndicesService and Node.java independently create TieredStoragePrefetchSettings instances, which registers duplicate settings update consumers with ClusterSettings. This could lead to inconsistent state and is a valid architectural concern. The fix of passing the supplier via constructor is the correct approach.	Medium
	Guard against null shard reference getShardOrNull(0) can return null if the shard is not yet assigned or initialized on the data node. Calling .store().directory() on a null shard will throw a NullPointerException. Add a non-null assertion or use getShard(0) to fail with a clearer error. server/src/internalClusterTest/java/org/opensearch/storage/WarmIndexBasicIT.java [112-116] FileCache fileCache = internalTestCluster.getDataNodeInstance(Node.class).fileCache(); IndexShard shard = internalTestCluster.getDataNodeInstance(IndicesService.class) .indexService(resolveIndex(INDEX_NAME)) .getShardOrNull(0); +assertNotNull("Expected shard 0 to be present on the data node", shard); Directory directory = unwrapToCompositeDirectory(shard.store().directory()); Suggestion importance[1-10]: 5 __ Why: getShardOrNull(0) can return null if the shard isn't yet assigned, causing a NullPointerException on the subsequent .store().directory() call. Adding a null assertion provides a clearer failure message in integration tests.	Low
General	Fix misleading exception message on unwrap failure The error message in the IllegalArgumentException reports the class name of the original directory argument, but after unwrapping through FilterDirectory layers, current may be a completely different (non-FSDirectory) type. The message should report current.getClass().getName() to accurately identify the innermost unwrapped type that failed the check. server/src/main/java/org/opensearch/storage/utils/DirectoryUtils.java [55-64] public static FSDirectory unwrapFSDirectory(Directory directory) { Directory current = directory; while (current instanceof FilterDirectory) { current = ((FilterDirectory) current).getDelegate(); } if (current instanceof FSDirectory) { return (FSDirectory) current; } - throw new IllegalArgumentException("Expected FSDirectory but got: " + directory.getClass().getName()); + throw new IllegalArgumentException("Expected FSDirectory but got: " + current.getClass().getName()); } Suggestion importance[1-10]: 4 __ Why: The error message reports directory.getClass().getName() (the original wrapped directory) instead of current.getClass().getName() (the innermost unwrapped type). The improved code accurately identifies the actual failing type, making debugging easier.	Low

Suggestions up to commit 7463a62

Category	Suggestion	Impact
Possible issue	Avoid duplicate settings instance creation IndicesService creates its own TieredStoragePrefetchSettings instance independently from the one created in Node.java, resulting in two separate instances registered against clusterService.getClusterSettings(). This can cause duplicate settings registration errors or inconsistent settings state. The TieredStoragePrefetchSettings instance (or its supplier) should be injected into IndicesService rather than constructed internally. server/src/main/java/org/opensearch/indices/IndicesService.java [516-521] -if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)) { - final TieredStoragePrefetchSettings prefetchSettings = new TieredStoragePrefetchSettings(clusterService.getClusterSettings()); - this.tieredStoragePrefetchSettingsSupplier = () -> prefetchSettings; -} else { - this.tieredStoragePrefetchSettingsSupplier = () -> null; -} +// Accept tieredStoragePrefetchSettingsSupplier as a constructor parameter instead of creating a new instance here +this.tieredStoragePrefetchSettingsSupplier = tieredStoragePrefetchSettingsSupplier; Suggestion importance[1-10]: 7 __ Why: IndicesService creates its own TieredStoragePrefetchSettings independently from Node.java, potentially causing duplicate settings registration against clusterService.getClusterSettings(). Injecting the supplier as a constructor parameter would be cleaner and avoid this issue.	Medium
General	Fix misleading exception message on unwrap failure The error message in the IllegalArgumentException reports the class name of the original directory argument, but after unwrapping, the actual non-FSDirectory type found is current. This makes debugging harder since the reported class may be a FilterDirectory wrapper rather than the actual problematic type. The message should report current.getClass().getName() to show the innermost unwrapped type. server/src/main/java/org/opensearch/storage/utils/DirectoryUtils.java [55-64] public static FSDirectory unwrapFSDirectory(Directory directory) { Directory current = directory; while (current instanceof FilterDirectory) { current = ((FilterDirectory) current).getDelegate(); } if (current instanceof FSDirectory) { return (FSDirectory) current; } - throw new IllegalArgumentException("Expected FSDirectory but got: " + directory.getClass().getName()); + throw new IllegalArgumentException("Expected FSDirectory but got: " + current.getClass().getName()); } Suggestion importance[1-10]: 5 __ Why: The error message uses directory.getClass().getName() (the original wrapper) instead of current.getClass().getName() (the actual innermost type), making debugging harder when the input is a FilterDirectory wrapping a non-FSDirectory.	Low
	Strengthen feature-flag conditional wiring test coverage The REMOTE_DOWNLOAD thread pool is registered unconditionally in ThreadPool.java (not gated by the feature flag), so testTieredStorageWiringWithFeatureFlag does not actually verify that the thread pool is only present when the feature flag is enabled. Consider adding a complementary assertion that verifies the TierActionMetrics binding is absent (or throws) when the feature flag is disabled, to properly test the conditional wiring. server/src/test/java/org/opensearch/node/NodeTests.java [427-443] public void testTieredStorageWiringWithFeatureFlag() throws Exception { Settings warmRoleSettings = addRoles( baseSettings().put(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG, true) .put(Node.NODE_SEARCH_CACHE_SIZE_SETTING.getKey(), "1gb") .build(), Set.of(DiscoveryNodeRole.WARM_ROLE) ); List<Class<? extends Plugin>> plugins = basePlugins(); try (MockNode mockNode = new MockNode(warmRoleSettings, plugins)) { assertNotNull(mockNode); - // Verify TierActionMetrics was bound in Guice assertNotNull(mockNode.injector().getInstance(TierActionMetrics.class)); - // Verify remote_download thread pool exists ThreadPool threadPool = mockNode.injector().getInstance(ThreadPool.class); assertNotNull(threadPool.executor(ThreadPool.Names.REMOTE_DOWNLOAD)); } + + // Verify TierActionMetrics is NOT bound when feature flag is disabled + Settings noFlagSettings = addRoles( + baseSettings().put(Node.NODE_SEARCH_CACHE_SIZE_SETTING.getKey(), "1gb").build(), + Set.of(DiscoveryNodeRole.WARM_ROLE) + ); + try (MockNode mockNode = new MockNode(noFlagSettings, basePlugins())) { + expectThrows(Exception.class, () -> mockNode.injector().getInstance(TierActionMetrics.class)); + } } Suggestion importance[1-10]: 3 __ Why: The suggestion to add a negative test case (feature flag disabled) is valid for completeness, but the REMOTE_DOWNLOAD thread pool is unconditionally registered, so the test doesn't fully validate conditional behavior. The improvement is minor and the improved_code adds useful but non-critical coverage.	Low

Suggestions up to commit 1ed6f00

Category	Suggestion	Impact
Possible issue	Avoid duplicate settings instance creation IndicesService creates its own TieredStoragePrefetchSettings instance independently from the one created in Node.java, resulting in two separate instances registered with clusterService.getClusterSettings(). This can cause duplicate settings update consumer registrations and inconsistent state. The TieredStoragePrefetchSettings instance (or its supplier) should be injected into IndicesService rather than constructed internally. server/src/main/java/org/opensearch/indices/IndicesService.java [516-521] -if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)) { - final TieredStoragePrefetchSettings prefetchSettings = new TieredStoragePrefetchSettings(clusterService.getClusterSettings()); - this.tieredStoragePrefetchSettingsSupplier = () -> prefetchSettings; -} else { - this.tieredStoragePrefetchSettingsSupplier = () -> null; -} +// Accept tieredStoragePrefetchSettingsSupplier as a constructor parameter instead +// and assign: this.tieredStoragePrefetchSettingsSupplier = tieredStoragePrefetchSettingsSupplier; Suggestion importance[1-10]: 7 __ Why: IndicesService creates its own TieredStoragePrefetchSettings independently from Node.java, potentially causing duplicate settings update consumer registrations with clusterService.getClusterSettings(). Injecting the supplier as a constructor parameter would be cleaner and avoid this duplication.	Medium
	Add null check for shard before use getShardOrNull(0) may return null if the shard is not yet assigned or initialized on the data node, which would cause a NullPointerException when calling shard.store().directory(). The result should be asserted non-null before use. server/src/internalClusterTest/java/org/opensearch/storage/WarmIndexBasicIT.java [112-116] FileCache fileCache = internalTestCluster.getDataNodeInstance(Node.class).fileCache(); IndexShard shard = internalTestCluster.getDataNodeInstance(IndicesService.class) .indexService(resolveIndex(INDEX_NAME)) .getShardOrNull(0); +assertNotNull("Expected shard 0 to be present on data node", shard); Directory directory = unwrapToCompositeDirectory(shard.store().directory()); Suggestion importance[1-10]: 5 __ Why: getShardOrNull(0) can return null if the shard isn't yet assigned, which would cause a NullPointerException on shard.store().directory(). Adding an assertNotNull guard improves test robustness.	Low
General	Fix misleading exception message on unwrap failure The error message in the IllegalArgumentException reports the class name of the original directory argument, but after unwrapping through FilterDirectory layers, current may be a different (non-FSDirectory) type. The message should report current.getClass().getName() to accurately identify the actual unwrapped type that caused the failure. server/src/main/java/org/opensearch/storage/utils/DirectoryUtils.java [55-64] public static FSDirectory unwrapFSDirectory(Directory directory) { Directory current = directory; while (current instanceof FilterDirectory) { current = ((FilterDirectory) current).getDelegate(); } if (current instanceof FSDirectory) { return (FSDirectory) current; } - throw new IllegalArgumentException("Expected FSDirectory but got: " + directory.getClass().getName()); + throw new IllegalArgumentException("Expected FSDirectory but got: " + current.getClass().getName()); } Suggestion importance[1-10]: 4 __ Why: The error message reports directory.getClass().getName() (the original wrapped directory) instead of current.getClass().getName() (the actual unwrapped type that failed the check), making debugging harder when the directory is wrapped in FilterDirectory layers.	Low

[github-actions]

❌ Gradle check result for 61c7e09: 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]

Persistent review updated to latest commit faa7672

[github-actions]

❌ Gradle check result for faa7672: 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]

Persistent review updated to latest commit 47a3138

[github-actions]

Persistent review updated to latest commit 6a6971f

[github-actions]

✅ Gradle check result for 6a6971f: SUCCESS

[github-actions]

Persistent review updated to latest commit 1ed6f00

[github-actions]

✅ Gradle check result for 1ed6f00: SUCCESS

[github-actions]

Persistent review updated to latest commit 7463a62

[github-actions]

❌ Gradle check result for 7463a62: 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]

Persistent review updated to latest commit f1f484d

[github-actions]

✅ Gradle check result for f1f484d: SUCCESS

[github-actions]

Persistent review updated to latest commit 9af0520

[github-actions]

Persistent review updated to latest commit b4ad170

[github-actions]

❌ Gradle check result for b4ad170: 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]

Persistent review updated to latest commit b4ad170

[github-actions]

✅ Gradle check result for b4ad170: SUCCESS