Add skeleton structure for tiered-storage module

[GeekGlider]

Description

Adds the skeleton structure for the full tiered-storage module, locking down all design decisions upfront so that future implementation PRs don't need to debate naming or structure.

What's locked down in this PR:

1. Package names — org.opensearch.storage.{slowlogs, directory, indexinput, common, common.tiering, tiering, metrics, utils, action.tiering, action.tiering.status}
2. Class names — All 45+ classes with their inheritance hierarchy (extends/implements)
3. Transport action names — indices:admin/_tier/hot_to_warm, indices:admin/_tier/warm_to_hot, indices:admin/_tier/cancel, indices:admin/_tier/get, cluster:admin/_tier/all
4. REST endpoint names — POST /{index}/_tier/warm, POST /{index}/_tier/hot, POST /_tier/_cancel/{index}, GET /{index}/_tier, GET /_tier/all
5. Cluster settings with defaults — 4 tiering settings (cluster.tiering.*), 2 prefetch settings (tiering.service.prefetch.*), 10 slow log settings (index.tiered.storage.slowlog.*)
6. Serialized field names — All Writeable request/response classes with their field declarations
7. Metric names — migration_successful, migration_rejection_reason, migration_latency with tags node_id, index_name, tier_type, rejection_reason

What's NOT in this PR:

• No implementation logic — all method bodies throw UnsupportedOperationException("Not yet implemented")
• No tests — nothing to test since there's no implementation
• Implementation will be added in subsequent PRs following the plan (PR2: slow logs, PR3: directory layer, PR4: index input, etc.)

Related Issues

Part of the tiered-storage open source plan.

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]

Failed to generate code suggestions for PR

[github-actions]

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

Failed to generate code suggestions for PR

[github-actions]

❌ Gradle check result for 52a6cf3: null

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]

Failed to generate code suggestions for PR

[github-actions]

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

PR Reviewer Guide 🔍

(Review updated until commit 5133192)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

📝 TODO sections modules/tiered-storage/src/main/java/org/opensearch/storage/slowlogs/TieredStorageQueryMetricService.java [160]: Has to emit as part of node stats

🔀 Multiple PR themes Sub-PR theme: Tiering service core and settings skeleton Relevant files: modules/tiered-storage/src/main/java/org/opensearch/storage/metrics/TierActionMetrics.java modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringUtils.java modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringRejectionException.java modules/tiered-storage/src/main/java/org/opensearch/storage/tiering/TieringService.java modules/tiered-storage/src/main/java/org/opensearch/storage/tiering/HotToWarmTieringService.java modules/tiered-storage/src/main/java/org/opensearch/storage/tiering/WarmToHotTieringService.java Sub-PR theme: Tiering transport actions and REST API skeleton Relevant files: modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/IndexTieringRequest.java modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/CancelTieringRequest.java modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/TransportTierAction.java modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/TransportCancelTierAction.java modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/model/GetTieringStatusRequest.java modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/model/ListTieringStatusRequest.java modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/model/TieringStatus.java modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/transport/TransportGetTieringStatusAction.java modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/transport/TransportListTieringStatusAction.java Sub-PR theme: Directory, IndexInput, prefetch, and slow log skeleton Relevant files: modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/BlockFetchRequest.java modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/BlockIndexInput.java modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/SwitchableIndexInput.java modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/OnDemandPrefetchBlockSnapshotIndexInput.java modules/tiered-storage/src/main/java/org/opensearch/storage/directory/OSBlockHotDirectory.java modules/tiered-storage/src/main/java/org/opensearch/storage/common/BlockTransferManager.java modules/tiered-storage/src/main/java/org/opensearch/storage/prefetch/TieredStoragePrefetchSettings.java modules/tiered-storage/src/main/java/org/opensearch/storage/slowlogs/TieredStorageQueryMetricService.java modules/tiered-storage/src/main/java/org/opensearch/storage/slowlogs/TieredStorageSearchSlowLog.java modules/tiered-storage/src/main/java/org/opensearch/storage/slowlogs/PrefetchStats.java modules/tiered-storage/src/main/java/org/opensearch/storage/slowlogs/TieredStoragePerQueryMetricImpl.java

⚡ Recommended focus areas for review Typo in Setting Key The setting keys H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY in TieringUtils contain a typo ("TIEIRNG" instead of "TIERING"). Since these are serialized setting names locked down in this PR, fixing the typo now is critical before any implementation or data is persisted. protected final ConcurrentMap<Long, TieredStoragePerQueryMetric> metricCollectors = new ConcurrentHashMap<>(); /** * Map of task id + shard id to set of collectors, providing a way to look up all collectors for a given task/shard. We need both * as the same parent task may have multiple shards on the same node. For concurrent segment search there will be multiple collectors * per task/shard combination as each slice (thread) creates its own collector. The same thread can process multiple slices for the same * or for different queries so it does not come into the picture here. */ protected final ConcurrentMap<String, Set<TieredStoragePerQueryMetric>> taskIdToQueryPhaseCollectorMap = new ConcurrentHashMap<>(); /** Map of task ID and shard ID to set of fetch phase metric collectors. */ protected final ConcurrentMap<String, Set<TieredStoragePerQueryMetric>> taskIdToFetchPhaseCollectorMap = new ConcurrentHashMap<>(); private final PrefetchStatsHolder prefetchStats = new PrefetchStatsHolder(); /** * Maximum number of metric collectors for single warm node */ private static final int MAX_PER_QUERY_COLLECTOR_SIZE = 1000; /** * Private constructor to keep class Singleton */ private TieredStorageQueryMetricService() {} Nullable Fields Risk The placeholder constructor assigns null to many final fields (fileCache, fullFilePath, localDirectory, remoteDirectory, transferManager, clones, etc.). If any method is accidentally called before the real constructor is added, NullPointerExceptions will occur silently rather than failing fast. Consider adding a guard or making the placeholder constructor private/package-private. SwitchableIndexInput(String resourceDescription) { super(resourceDescription); this.fileCache = null; this.fullFilePath = null; this.switchableFilePath = null; this.fileName = null; this.fileLength = 0; this.offset = 0; this.localDirectory = null; this.remoteDirectory = null; this.transferManager = null; this.isClone = false; this.clones = null; this.tieredStoragePrefetchSettingsSupplier = null; this.threadPool = null; } Incomplete Validation The validate() method unconditionally returns null, meaning no validation is performed even in the skeleton. If an invalid targetTier value is passed (e.g., not "HOT" or "WARM"), it will silently be accepted. At minimum, a TODO or placeholder validation should be noted. @Override public ActionRequestValidationException validate() { return null; } Type Mismatch The directory field in BlockFetchRequest is declared as Directory (Lucene interface), but the Builder uses FSDirectory (a concrete subclass). The constructor assigns builder.directory (FSDirectory) to this.directory (Directory), which is fine for assignment, but getDirectory() returns Directory. This inconsistency may cause issues for callers expecting FSDirectory. Consider aligning the types. private final Directory directory; private final String fileName; private final String blockFileName; private final long blockStart; private final long blockSize; private final Path filePath; private BlockFetchRequest(Builder builder) { this.fileName = builder.fileName; this.blockFileName = builder.blockFileName; this.filePath = builder.directory.getDirectory().resolve(blockFileName); this.directory = builder.directory; this.blockSize = builder.blockSize; this.blockStart = builder.blockStart; } Typo in Setting Key Both H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY ("TIEIRNG") and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY ("TIEIRNG") contain a typo. Since this PR locks down setting names, this typo will be persisted in cluster state and configuration files, making it hard to fix later without a breaking change. public static final String H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; /** Setting for maximum concurrent hot-to-warm tiering requests. */ public static final Setting<Integer> H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, DEFAULT_H2W_MAX_CONCURRENT_TIERING_REQUESTS, 0, 1000, Setting.Property.Dynamic, Setting.Property.NodeScope ); private static final int DEFAULT_W2H_MAX_CONCURRENT_TIERING_REQUESTS = 10; /** Setting key for maximum concurrent warm-to-hot tiering requests. */ public static final String W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; /** Setting for maximum concurrent warm-to-hot tiering requests. */ public static final Setting<Integer> W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, DEFAULT_W2H_MAX_CONCURRENT_TIERING_REQUESTS, 0, 1000, Setting.Property.Dynamic, Setting.Property.NodeScope );

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5133192

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix integer overflow in byte constant calculation The constant GB_IN_BYTES is computed as 1024 * 1024 * 1024, which is an int multiplication that overflows before being assigned to a long. This results in a negative value for GB_IN_BYTES and consequently an incorrect value for MIN_DISK_SPACE_BUFFER. Use a long literal to avoid integer overflow. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringServiceValidator.java [20-21] -private static final long GB_IN_BYTES = 1024 * 1024 * 1024; +private static final long GB_IN_BYTES = 1024L * 1024L * 1024L; private static final long MIN_DISK_SPACE_BUFFER = 20 * GB_IN_BYTES; Suggestion importance[1-10]: 8 __ Why: 1024 * 1024 * 1024 is computed as int arithmetic and overflows (resulting in a negative value) before being assigned to long. Using 1024L * 1024L * 1024L ensures the multiplication is done in long arithmetic, producing the correct value of 1 GB.	Medium
	Add null check before dereferencing builder directory The constructor accesses builder.directory.getDirectory() without a null check. If builder.directory is null (e.g., when build() is called without setting the directory), this will throw a NullPointerException. Add a null check or validation in the build() method before constructing the object. modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/BlockFetchRequest.java [31-38] private BlockFetchRequest(Builder builder) { + if (builder.directory == null) { + throw new IllegalStateException("directory must be set before building BlockFetchRequest"); + } this.fileName = builder.fileName; this.blockFileName = builder.blockFileName; this.filePath = builder.directory.getDirectory().resolve(blockFileName); this.directory = builder.directory; this.blockSize = builder.blockSize; this.blockStart = builder.blockStart; } Suggestion importance[1-10]: 6 __ Why: The builder.directory could be null if build() is called without setting the directory, causing a NullPointerException. Adding a null check in the constructor or build() method would prevent this. This is a valid defensive programming concern, though the builder pattern typically relies on callers to set required fields.	Low
General	Fix typo in public constant names The constant names H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS, W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS all contain a typo: "TIEIRNG" instead of "TIERING". Since these are public API constants referenced in TieredStoragePlugin.java, fixing this now (before implementation) avoids a breaking change later. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringUtils.java [44-66] -public static final String H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; +public static final String H2W_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; /** Setting for maximum concurrent hot-to-warm tiering requests. */ -public static final Setting<Integer> H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( +public static final Setting<Integer> H2W_MAX_CONCURRENT_TIERING_REQUESTS = Setting.intSetting( ... -public static final String W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; +public static final String W2H_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; /** Setting for maximum concurrent warm-to-hot tiering requests. */ -public static final Setting<Integer> W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( +public static final Setting<Integer> W2H_MAX_CONCURRENT_TIERING_REQUESTS = Setting.intSetting( Suggestion importance[1-10]: 7 __ Why: The constants H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS, W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS all contain the typo "TIEIRNG" instead of "TIERING". Since these are public API constants already referenced in TieredStoragePlugin.java, fixing the typo now avoids a breaking rename later.	Medium
	Enforce singleton pattern with private constructor GetTieringStatusAction has a public constructor, but unlike ListTieringStatusAction, CancelTieringAction, HotToWarmTierAction, and WarmToHotTierAction, it should use a private constructor to enforce the singleton pattern. A public constructor allows multiple instances to be created, which is inconsistent with the singleton design used across all other action types in this module. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/GetTieringStatusAction.java [23-25] /** Constructs a new GetTieringStatusAction. */ -public GetTieringStatusAction() { +private GetTieringStatusAction() { super(NAME, GetTieringStatusResponse::new); } Suggestion importance[1-10]: 7 __ Why: The public constructor in GetTieringStatusAction is inconsistent with the private constructors used in all other action types (ListTieringStatusAction, CancelTieringAction, HotToWarmTierAction, WarmToHotTierAction), breaking the singleton pattern and allowing multiple instances to be created.	Medium
	Avoid partial stream consumption before throwing exception The StreamInput constructor calls super(in) which reads from the stream, but then immediately throws UnsupportedOperationException. This is inconsistent with other skeleton constructors in this PR (e.g., GetTieringStatusRequest, ListTieringStatusRequest) that follow the same pattern. However, since this constructor is used for deserialization during transport, any attempt to deserialize a CancelTieringRequest over the wire will fail after partially consuming the stream, potentially corrupting the stream state. This should be documented clearly or the super(in) call should be guarded. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/CancelTieringRequest.java [39-42] public CancelTieringRequest(StreamInput in) throws IOException { - super(in); + // Note: super(in) intentionally omitted until implementation is complete + // to avoid partial stream consumption before UnsupportedOperationException + super(); throw new UnsupportedOperationException("Not yet implemented"); } Suggestion importance[1-10]: 3 __ Why: While the concern about partial stream consumption is valid in theory, this is a skeleton/placeholder PR where all transport methods throw UnsupportedOperationException. The suggested fix of calling super() instead of super(in) could introduce other issues. The pattern of super(in) followed by the exception is consistent across many files in this PR.	Low
	Prevent resource leak from partial superclass initialization Both constructors call super(delegate) before throwing UnsupportedOperationException. The FilterDirectory superclass constructor may acquire resources or register the delegate. If the constructor throws immediately after, the delegate directory may not be properly closed, potentially causing resource leaks. Consider not calling super(delegate) until the implementation is ready, or ensure the delegate is closed in a finally block. modules/tiered-storage/src/main/java/org/opensearch/storage/directory/OSBlockHotDirectory.java [61-88] public OSBlockHotDirectory( Directory delegate, Directory remoteDirectory, IndexSettings indexSettings, Supplier<ThreadPool> threadPoolSupplier ) throws IOException { super(delegate); + // TODO: Full initialization will be added in the implementation PR. + // Ensure delegate is closed if initialization fails. throw new UnsupportedOperationException("Not yet implemented"); } -... - -public OSBlockHotDirectory( - Directory delegate, - Directory remoteDirectory, - IndexSettings indexSettings, - BlockTransferManager blockTransferManager -) throws IOException { - super(delegate); - throw new UnsupportedOperationException("Not yet implemented"); -} - Suggestion importance[1-10]: 3 __ Why: The concern about resource leaks from FilterDirectory superclass initialization is valid in principle, but the improved_code is essentially the same as the existing_code with just a comment added. Since this is a placeholder PR, the actual resource management will be handled in the implementation PR.	Low

---

Previous suggestions

Suggestions up to commit 5cd1173

Category	Suggestion	Impact
Possible issue	Fix integer overflow in byte constant calculation The expression 1024 * 1024 * 1024 is computed as int arithmetic before being assigned to a long, which will overflow since the result (1,073,741,824) fits in an int but 20 * GB_IN_BYTES (21,474,836,480) does not. The literal should use the L suffix to force long arithmetic. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringServiceValidator.java [20-21] -private static final long GB_IN_BYTES = 1024 * 1024 * 1024; +private static final long GB_IN_BYTES = 1024L * 1024L * 1024L; private static final long MIN_DISK_SPACE_BUFFER = 20 * GB_IN_BYTES; Suggestion importance[1-10]: 8 __ Why: 1024 * 1024 * 1024 is evaluated as int arithmetic (result is 1,073,741,824 which fits in int), but 20 * GB_IN_BYTES would overflow int if GB_IN_BYTES were computed as int. Using the L suffix ensures correct long arithmetic and prevents a subtle overflow bug.	Medium
	Enforce singleton pattern with private constructor GetTieringStatusAction has a public constructor, but the singleton pattern used here (with a public static final INSTANCE) typically requires a private constructor to prevent external instantiation. This is inconsistent with ListTieringStatusAction, CancelTieringAction, HotToWarmTierAction, and WarmToHotTierAction, which all use private constructors. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/GetTieringStatusAction.java [23-25] /** Constructs a new GetTieringStatusAction. */ -public GetTieringStatusAction() { +private GetTieringStatusAction() { super(NAME, GetTieringStatusResponse::new); } Suggestion importance[1-10]: 7 __ Why: The public constructor on GetTieringStatusAction is inconsistent with the singleton pattern used by all other action types in this PR (ListTieringStatusAction, CancelTieringAction, etc.), which all use private constructors. This is a real design inconsistency that could allow unintended external instantiation.	Medium
	Add null validation for required builder fields The constructor accesses builder.directory without a null check, which will throw a NullPointerException if directory was not set on the builder before calling build(). Add a null check or validation in the build() method to ensure required fields like directory and blockFileName are set before constructing the object. modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/BlockFetchRequest.java [31-38] private BlockFetchRequest(Builder builder) { + if (builder.directory == null) { + throw new IllegalStateException("directory must be set"); + } + if (builder.blockFileName == null) { + throw new IllegalStateException("blockFileName must be set"); + } this.fileName = builder.fileName; this.blockFileName = builder.blockFileName; this.filePath = builder.directory.getDirectory().resolve(blockFileName); this.directory = builder.directory; this.blockSize = builder.blockSize; this.blockStart = builder.blockStart; } Suggestion importance[1-10]: 6 __ Why: The constructor accesses builder.directory without null checking, which would cause a NullPointerException if directory is not set. The validation is a legitimate improvement, though this is a scaffold PR where the build() method will likely add validation later.	Low
	Initialize clones map to prevent NullPointerException The clones field is initialized to null in the placeholder constructor and is never assigned a real ConcurrentHashMap. Any future code that calls methods on clones (e.g., clones.put(...)) will throw a NullPointerException. The field should be initialized to a new ConcurrentHashMap<>() in the placeholder constructor to avoid this, or at minimum documented clearly that it must be initialized before use. modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/SwitchableIndexInput.java [103] -private final ConcurrentMap<SwitchableIndexInput, Boolean> clones; +this.clones = new ConcurrentHashMap<>(); Suggestion importance[1-10]: 5 __ Why: The clones field is set to null in the placeholder constructor, which could cause NullPointerException when the real implementation uses it. However, since this is a scaffold PR with placeholder constructors, the real constructor will properly initialize this field in the implementation PR.	Low
General	Fix typo in public constant names Both H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY (and their corresponding Setting constants) contain a typo: "TIEIRNG" should be "TIERING". Since these are public API constants used as setting keys, fixing the typo now (before implementation) avoids a breaking change later. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringUtils.java [44-59] -public static final String H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; +public static final String H2W_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; /** Setting for maximum concurrent hot-to-warm tiering requests. */ -public static final Setting<Integer> H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( +public static final Setting<Integer> H2W_MAX_CONCURRENT_TIERING_REQUESTS = Setting.intSetting( ... -public static final String W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; +public static final String W2H_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; /** Setting for maximum concurrent warm-to-hot tiering requests. */ -public static final Setting<Integer> W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( +public static final Setting<Integer> W2H_MAX_CONCURRENT_TIERING_REQUESTS = Setting.intSetting( Suggestion importance[1-10]: 7 __ Why: The constants H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS, W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS all contain the typo "TIEIRNG" instead of "TIERING". Fixing this now before implementation avoids a breaking API change later, and these constants are already referenced in TieredStoragePlugin.java.	Medium
	Make response list field immutable The tieringStatusList field is mutable and exposed directly via getTieringStatusList(). It should be stored as an unmodifiable list or a defensive copy should be made in the constructor and getter to prevent external mutation of the response's internal state. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/model/ListTieringStatusResponse.java [21] -private List<TieringStatus> tieringStatusList; +private final List<TieringStatus> tieringStatusList; Suggestion importance[1-10]: 3 __ Why: Adding final to tieringStatusList prevents reassignment but does not make the list itself unmodifiable. The suggestion's improved_code only adds final, which is a minor improvement but doesn't fully address the mutability concern described in the suggestion content.	Low
	Warn against premature use of unimplemented stream constructors The StreamInput constructor calls super(in) which reads parent state from the stream, but then immediately throws UnsupportedOperationException. If this constructor is invoked during deserialization (e.g., when the request is received over transport), it will cause a runtime failure after partially consuming the stream. The same pattern exists in GetTieringStatusRequest, ListTieringStatusRequest, and IndexTieringRequest. Consider adding a clear comment or guard to prevent accidental use until implemented. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/CancelTieringRequest.java [39-42] public CancelTieringRequest(StreamInput in) throws IOException { super(in); + // TODO: Implement deserialization in the implementation PR. + // WARNING: Do not register this as a transport reader until implemented. throw new UnsupportedOperationException("Not yet implemented"); } Suggestion importance[1-10]: 2 __ Why: This is a scaffold PR where all stream constructors intentionally throw UnsupportedOperationException. Adding a comment is a minor documentation improvement but doesn't address a real bug in the current state of the code.	Low

Suggestions up to commit 3dbfec7

Category	Suggestion	Impact
General	Fix typo in public constant names The constant names H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS, W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS all contain a typo: "TIEIRNG" should be "TIERING". Since these are public API constants referenced in TieredStoragePlugin, fixing this now (before the implementation PR) avoids a breaking rename later. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringUtils.java [44-66] -public static final String H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; +public static final String H2W_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; /** Setting for maximum concurrent hot-to-warm tiering requests. */ -public static final Setting<Integer> H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( +public static final Setting<Integer> H2W_MAX_CONCURRENT_TIERING_REQUESTS = Setting.intSetting( ... -public static final String W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; +public static final String W2H_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; /** Setting for maximum concurrent warm-to-hot tiering requests. */ -public static final Setting<Integer> W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( +public static final Setting<Integer> W2H_MAX_CONCURRENT_TIERING_REQUESTS = Setting.intSetting( Suggestion importance[1-10]: 7 __ Why: The public constants H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS contain a clear typo ("TIEIRNG" instead of "TIERING"). Since these are public API constants already referenced in TieredStoragePlugin, fixing the typo now avoids a breaking rename in a future PR.	Medium
	Enforce singleton pattern with private constructor GetTieringStatusAction has a public constructor, but unlike ListTieringStatusAction, CancelTieringAction, HotToWarmTierAction, and WarmToHotTierAction, it should use a private constructor to enforce the singleton pattern. A public constructor allows multiple instances to be created, which is inconsistent with the singleton design used across all other action types in this module. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/GetTieringStatusAction.java [23-25] /** Constructs a new GetTieringStatusAction. */ -public GetTieringStatusAction() { +private GetTieringStatusAction() { super(NAME, GetTieringStatusResponse::new); } Suggestion importance[1-10]: 7 __ Why: The public constructor on GetTieringStatusAction is inconsistent with all other action types in this module (ListTieringStatusAction, CancelTieringAction, HotToWarmTierAction, WarmToHotTierAction) which all use private constructors to enforce the singleton pattern. This is a real design inconsistency that could lead to multiple instances being created.	Medium
	Remove redundant duplicate field from parent class TransportCancelTierAction stores a redundant local copy of indexNameExpressionResolver that is already available via the parent class TransportClusterManagerNodeAction. This duplicate field wastes memory and may cause confusion. Remove the local field and use the inherited one instead. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/TransportCancelTierAction.java [41-73] -private final IndexNameExpressionResolver indexNameExpressionResolver; -... -this.indexNameExpressionResolver = indexNameExpressionResolver; +// Remove the local field declaration: +// private final IndexNameExpressionResolver indexNameExpressionResolver; +// And remove the assignment in the constructor: +// this.indexNameExpressionResolver = indexNameExpressionResolver; + Suggestion importance[1-10]: 4 __ Why: The local indexNameExpressionResolver field duplicates one already available from the parent TransportClusterManagerNodeAction. This is a minor code quality issue that adds unnecessary memory usage and potential confusion, but has no functional impact.	Low
	Restrict field visibility and mutability The field tieredStoragePrefetchSettingsSupplier is declared public and non-final, which breaks encapsulation and allows external mutation. It should be private (or at most protected) and final to match the access pattern of the other fields in this class and to prevent unintended modification. modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/OnDemandPrefetchBlockSnapshotIndexInput.java [33] /** Supplier for prefetch settings. */ -public Supplier<TieredStoragePrefetchSettings> tieredStoragePrefetchSettingsSupplier; +protected final Supplier<TieredStoragePrefetchSettings> tieredStoragePrefetchSettingsSupplier; Suggestion importance[1-10]: 4 __ Why: The tieredStoragePrefetchSettingsSupplier field is public and non-final, breaking encapsulation and allowing external mutation. Changing it to protected final aligns with the other fields in the class and is a valid encapsulation improvement.	Low
	Make response field immutable to prevent mutation The tieringStatusList field is mutable and not defensively copied, which means callers could modify the internal state of the response after construction. It should be stored as an immutable list or at least as a defensive copy to prevent unintended mutation. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/model/ListTieringStatusResponse.java [21] -private List<TieringStatus> tieringStatusList; +private final List<TieringStatus> tieringStatusList; Suggestion importance[1-10]: 4 __ Why: Adding final to tieringStatusList prevents reassignment of the field but does not make the list itself immutable. The suggestion partially addresses the concern but doesn't fully prevent mutation of the list contents, making it a minor improvement.	Low
Possible issue	Add null check for required builder field The constructor accesses builder.directory without a null check. If directory is not set on the builder before calling build(), this will throw a NullPointerException. Add a null check or validation in the build() method to ensure required fields are set. modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/BlockFetchRequest.java [31-38] private BlockFetchRequest(Builder builder) { + if (builder.directory == null) { + throw new IllegalStateException("directory must be set before building BlockFetchRequest"); + } this.fileName = builder.fileName; this.blockFileName = builder.blockFileName; this.filePath = builder.directory.getDirectory().resolve(blockFileName); this.directory = builder.directory; this.blockSize = builder.blockSize; this.blockStart = builder.blockStart; } Suggestion importance[1-10]: 6 __ Why: The constructor accesses builder.directory without null validation, which would cause a NullPointerException at runtime if directory is not set. Adding a guard in the constructor or build() method is a valid defensive programming improvement.	Low
	Prevent integer overflow in constant computation The constant GB_IN_BYTES is computed as 1024 * 1024 * 1024, which uses int arithmetic and will overflow before being assigned to a long. The result of 1024 * 1024 * 1024 is 1073741824 (fits in int), but 20 * GB_IN_BYTES would then be 20 * 1073741824L which is fine since GB_IN_BYTES is already a long. However, to be safe and explicit, the literal should use a L suffix to ensure long arithmetic throughout. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringServiceValidator.java [20-21] -private static final long GB_IN_BYTES = 1024 * 1024 * 1024; +private static final long GB_IN_BYTES = 1024L * 1024L * 1024L; private static final long MIN_DISK_SPACE_BUFFER = 20 * GB_IN_BYTES; Suggestion importance[1-10]: 5 __ Why: While 1024 * 1024 * 1024 does not actually overflow (it equals 1073741824, which fits in an int), using 1024L * 1024L * 1024L is a safer and more explicit practice to avoid potential issues if the value is ever changed. The suggestion is valid but the current code does not actually have a bug.	Low

Suggestions up to commit a81330c

Category	Suggestion	Impact
Possible issue	Fix integer overflow in byte constant calculation The constant GB_IN_BYTES is computed as 1024 * 1024 * 1024, which is an int multiplication that overflows before being assigned to a long. The result will be -1073741824 instead of 1073741824. Use a long literal to avoid integer overflow. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringServiceValidator.java [20-21] -private static final long GB_IN_BYTES = 1024 * 1024 * 1024; +private static final long GB_IN_BYTES = 1024L * 1024 * 1024; private static final long MIN_DISK_SPACE_BUFFER = 20 * GB_IN_BYTES; Suggestion importance[1-10]: 8 __ Why: The expression 1024 * 1024 * 1024 is computed as int arithmetic and overflows to a negative value before being assigned to long. Using 1024L as the first operand prevents this overflow and ensures GB_IN_BYTES holds the correct value, which directly affects MIN_DISK_SPACE_BUFFER.	Medium
	Enforce singleton pattern with private constructor GetTieringStatusAction has a public constructor, but the singleton pattern used here (with a static INSTANCE) typically requires a private constructor to prevent external instantiation. All other action types in this PR (CancelTieringAction, HotToWarmTierAction, WarmToHotTierAction, ListTieringStatusAction) correctly use private constructors. This inconsistency could allow multiple instances to be created. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/GetTieringStatusAction.java [23-25] /** Constructs a new GetTieringStatusAction. */ -public GetTieringStatusAction() { +private GetTieringStatusAction() { super(NAME, GetTieringStatusResponse::new); } Suggestion importance[1-10]: 7 __ Why: The public constructor on GetTieringStatusAction breaks the singleton pattern used consistently across all other action types in this PR (CancelTieringAction, HotToWarmTierAction, etc.), which all use private constructors. This is a real inconsistency that could allow unintended multiple instantiations.	Medium
	Guard against null directory in constructor The constructor accesses builder.directory without a null check. If directory is not set on the builder before calling build(), this will throw a NullPointerException. Add a null validation in the build() method or in the constructor before dereferencing builder.directory. modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/BlockFetchRequest.java [31-38] private BlockFetchRequest(Builder builder) { + if (builder.directory == null) { + throw new IllegalStateException("directory must be set before building BlockFetchRequest"); + } this.fileName = builder.fileName; this.blockFileName = builder.blockFileName; this.filePath = builder.directory.getDirectory().resolve(blockFileName); this.directory = builder.directory; this.blockSize = builder.blockSize; this.blockStart = builder.blockStart; } Suggestion importance[1-10]: 6 __ Why: The constructor dereferences builder.directory without a null check, which would cause a NullPointerException if directory is not set. The suggestion correctly identifies this issue and adds a guard, though this is a builder pattern where the caller is responsible for setting required fields.	Low
General	Fix typo in public constant names The constant names H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY, H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS, and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS all contain a typo: "TIEIRNG" should be "TIERING". Since these are public API constants, fixing the typo now (before the implementation PR) avoids a breaking rename later. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringUtils.java [44-66] -public static final String H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; -... -public static final String W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; -... -public static final Setting<Integer> H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( -... -public static final Setting<Integer> W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS = Setting.intSetting( +public static final String H2W_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; +public static final Setting<Integer> H2W_MAX_CONCURRENT_TIERING_REQUESTS = Setting.intSetting( + H2W_MAX_CONCURRENT_TIERING_REQUESTS_KEY, + DEFAULT_H2W_MAX_CONCURRENT_TIERING_REQUESTS, + 0, + 1000, + Setting.Property.Dynamic, + Setting.Property.NodeScope +); +public static final String W2H_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; +public static final Setting<Integer> W2H_MAX_CONCURRENT_TIERING_REQUESTS = Setting.intSetting( + W2H_MAX_CONCURRENT_TIERING_REQUESTS_KEY, + DEFAULT_W2H_MAX_CONCURRENT_TIERING_REQUESTS, + 0, + 1000, + Setting.Property.Dynamic, + Setting.Property.NodeScope +); + Suggestion importance[1-10]: 7 __ Why: The public constants H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS contain a typo ("TIEIRNG" instead of "TIERING"). Fixing this now before the implementation PR avoids a breaking API rename later, and the typo also appears in TieredStoragePlugin.java where these constants are referenced.	Medium
	Restrict direct map access to enforce size limits The metricCollectors map and the two taskId maps are declared protected in a singleton class, which allows subclasses or package-level code to directly mutate the maps, bypassing the intended MAX_PER_QUERY_COLLECTOR_SIZE limit and other invariants. These fields should be private to enforce encapsulation; the existing package-private accessor getMetricCollectors() already provides controlled read access for tests.* modules/tiered-storage/src/main/java/org/opensearch/storage/slowlogs/TieredStorageQueryMetricService.java [44-54] -protected final ConcurrentMap<Long, TieredStoragePerQueryMetric> metricCollectors = new ConcurrentHashMap<>(); +private final ConcurrentMap<Long, TieredStoragePerQueryMetric> metricCollectors = new ConcurrentHashMap<>(); +private final ConcurrentMap<String, Set<TieredStoragePerQueryMetric>> taskIdToQueryPhaseCollectorMap = new ConcurrentHashMap<>(); +private final ConcurrentMap<String, Set<TieredStoragePerQueryMetric>> taskIdToFetchPhaseCollectorMap = new ConcurrentHashMap<>(); Suggestion importance[1-10]: 5 __ Why: The protected visibility on the maps in a singleton class allows subclasses and package-level code to bypass the MAX_PER_QUERY_COLLECTOR_SIZE limit. Making them private improves encapsulation, though the existing package-private accessor already provides test access for metricCollectors.	Low
	Prevent partial initialization of data-transfer object The fields indexName, status, source, and target are mutable via setters, but TieringStatus is used as a data-transfer object that is also serialized. Making these fields final and removing the no-arg constructor and setters (or at least making the parameterized constructor the primary construction path) would prevent accidental partial initialization and improve correctness. At minimum, the no-arg constructor leaves all string fields as null, which could cause NullPointerException when the object is used before being fully populated. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/model/TieringStatus.java [55-60] private ShardLevelStatus shardLevelStatus = null; -private String indexName; -private String status; -private String source; -private String target; -private long startTime; +private final String indexName; +private final String status; +private final String source; +private final String target; +private final long startTime; Suggestion importance[1-10]: 3 __ Why: Making fields final would require removing the no-arg constructor and setters, which are used in the existing code (e.g., setShardLevelStatus). The suggestion conflicts with the current design that uses setters for partial population, making it impractical without a larger refactor.	Low
	Make response list field immutable The tieringStatusList field is mutable and exposed via getTieringStatusList() without defensive copying. It should be declared as an unmodifiable list or copied on construction and retrieval to prevent external mutation of the response's internal state. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/model/ListTieringStatusResponse.java [21] -private List<TieringStatus> tieringStatusList; +private final List<TieringStatus> tieringStatusList; Suggestion importance[1-10]: 3 __ Why: Adding final to tieringStatusList prevents reassignment but does not make the list itself unmodifiable. The suggestion only partially addresses the stated concern and the improved_code only adds final, which is a minor improvement without wrapping in Collections.unmodifiableList().	Low

Suggestions up to commit a30ef73

Category	Suggestion	Impact
Possible issue	Enforce singleton pattern with private constructor GetTieringStatusAction has a public constructor, but the singleton pattern used here (with a public static final INSTANCE) typically requires a private constructor to prevent external instantiation. This is inconsistent with the other action classes in this PR (e.g., CancelTieringAction, ListTieringStatusAction) which correctly use private constructors. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/GetTieringStatusAction.java [23-25] /** Constructs a new GetTieringStatusAction. */ -public GetTieringStatusAction() { +private GetTieringStatusAction() { super(NAME, GetTieringStatusResponse::new); } Suggestion importance[1-10]: 7 __ Why: The public constructor on GetTieringStatusAction breaks the singleton pattern and is inconsistent with other action classes like CancelTieringAction and ListTieringStatusAction that use private constructors. This is a real design issue that could allow unintended instantiation.	Medium
	Fix integer overflow in byte constant calculation The expression 1024 * 1024 * 1024 is computed as int multiplication before being assigned to a long, which will cause integer overflow since the result (1,073,741,824) fits in an int but 20 * GB_IN_BYTES (21,474,836,480) does not. Use a long literal to avoid overflow. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringServiceValidator.java [20-21] -private static final long GB_IN_BYTES = 1024 * 1024 * 1024; +private static final long GB_IN_BYTES = 1024L * 1024L * 1024L; private static final long MIN_DISK_SPACE_BUFFER = 20 * GB_IN_BYTES; Suggestion importance[1-10]: 7 __ Why: 1024 * 1024 * 1024 is evaluated as int arithmetic (result is 1,073,741,824 which fits in int), but 20 * GB_IN_BYTES would overflow int if GB_IN_BYTES were treated as int. Using 1024L ensures the computation stays in long arithmetic and avoids potential overflow bugs.	Medium
	Add null check before dereferencing builder field The constructor accesses builder.directory without a null check. If directory is not set on the builder before calling build(), this will throw a NullPointerException. Add a null validation in the build() method or in the constructor before dereferencing builder.directory. modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/BlockFetchRequest.java [31-38] private BlockFetchRequest(Builder builder) { + if (builder.directory == null) { + throw new IllegalStateException("directory must be set before building BlockFetchRequest"); + } this.fileName = builder.fileName; this.blockFileName = builder.blockFileName; this.filePath = builder.directory.getDirectory().resolve(blockFileName); this.directory = builder.directory; this.blockSize = builder.blockSize; this.blockStart = builder.blockStart; } Suggestion importance[1-10]: 6 __ Why: The constructor dereferences builder.directory without a null check, which would cause a NullPointerException if directory is not set. This is a valid defensive programming concern, though the builder pattern typically relies on callers to set required fields.	Low
	Fix uninitialized fields used in abstract method parameters The fields maxConcurrentTieringRequests and jvmActiveUsageThresholdPercent are declared as private in the abstract base class but are passed as parameters to the abstract method validateTieringRequest. They are never initialized in the constructor, so subclasses will always receive null for these values. They should either be initialized from settings in the constructor or changed to protected so subclasses can set them. modules/tiered-storage/src/main/java/org/opensearch/storage/tiering/TieringService.java [63-64] -private Integer maxConcurrentTieringRequests; -private Integer jvmActiveUsageThresholdPercent; +protected Integer maxConcurrentTieringRequests; +protected Integer jvmActiveUsageThresholdPercent; Suggestion importance[1-10]: 5 __ Why: The fields maxConcurrentTieringRequests and jvmActiveUsageThresholdPercent are never initialized in the constructor, so they will always be null when passed to validateTieringRequest. However, since the PR is a scaffold with unimplemented methods, this may be intentionally deferred to the implementation PR.	Low
General	Fix typo in public constant names The constant names H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY contain a typo ("TIEIRNG" instead of "TIERING"). Since these are public API constants, fixing the typo now (before implementation) avoids a breaking rename later. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringUtils.java [44-59] -public static final String H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; +public static final String H2W_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_hot_to_warm_requests"; ... -public static final String W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; +public static final String W2H_MAX_CONCURRENT_TIERING_REQUESTS_KEY = "cluster.tiering.max_concurrent_warm_to_hot_requests"; Suggestion importance[1-10]: 7 __ Why: The constant names H2W_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY and W2H_MAX_CONCURRENT_TIEIRNG_REQUESTS_KEY contain a clear typo ("TIEIRNG" instead of "TIERING"). Fixing this now before implementation prevents a breaking API rename later, and the same typo is also present in the Setting constant names.	Medium
	Restrict visibility of inner holder class PrefetchStatsHolder is declared public static final but its constructor is package-private. Since TieredStorageQueryMetricService is a singleton, PrefetchStatsHolder should not be publicly instantiable from outside the class. Consider making the class package-private or private to match the intended encapsulation. modules/tiered-storage/src/main/java/org/opensearch/storage/slowlogs/TieredStorageQueryMetricService.java [210-218] -public static final class PrefetchStatsHolder { +static final class PrefetchStatsHolder { /** Constructs a new PrefetchStatsHolder. */ PrefetchStatsHolder() {} final CounterMetric storedFieldsPrefetchSuccess = new CounterMetric(); final CounterMetric storedFieldsPrefetchFailure = new CounterMetric(); final CounterMetric docValuesPrefetchSuccess = new CounterMetric(); final CounterMetric docValuesPrefetchFailure = new CounterMetric(); Suggestion importance[1-10]: 4 __ Why: PrefetchStatsHolder is declared public static final but has a package-private constructor, creating an inconsistency. Making the class package-private improves encapsulation, though this is a minor style concern for a scaffold PR.	Low
	Make response list field immutable The tieringStatusList field is mutable and exposed directly via getTieringStatusList(). It should be stored as an unmodifiable list or a defensive copy should be made in both the constructor and the getter to prevent external mutation of the response's internal state. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/model/ListTieringStatusResponse.java [21] -private List<TieringStatus> tieringStatusList; +private final List<TieringStatus> tieringStatusList; Suggestion importance[1-10]: 3 __ Why: Adding final to tieringStatusList is a minor improvement but doesn't fully address mutability since the list contents can still be modified externally. The suggestion's improved_code only adds final without making the list unmodifiable, making it an incomplete fix.	Low

Suggestions up to commit e96ceb8

Category	Suggestion	Impact
Possible issue	Fix integer overflow in byte constant calculation The constant GB_IN_BYTES is computed as 1024 * 1024 * 1024, which is an int multiplication that overflows before being assigned to a long. This results in a negative value for GB_IN_BYTES and consequently an incorrect value for MIN_DISK_SPACE_BUFFER. At least one literal in the multiplication should be a long. modules/tiered-storage/src/main/java/org/opensearch/storage/common/tiering/TieringServiceValidator.java [20-21] -private static final long GB_IN_BYTES = 1024 * 1024 * 1024; +private static final long GB_IN_BYTES = 1024L * 1024 * 1024; private static final long MIN_DISK_SPACE_BUFFER = 20 * GB_IN_BYTES; Suggestion importance[1-10]: 8 __ Why: 1024 * 1024 * 1024 is computed as int arithmetic and overflows to a negative value before being assigned to long, making GB_IN_BYTES incorrect. Using 1024L forces long arithmetic and fixes the overflow bug.	Medium
	Enforce singleton pattern with private constructor GetTieringStatusAction has a public constructor, but the singleton pattern used here (with a public static final INSTANCE) typically requires a private constructor to prevent external instantiation. Unlike ListTieringStatusAction, CancelTieringAction, HotToWarmTierAction, and WarmToHotTierAction which all use private constructors, this class exposes a public constructor that breaks the singleton guarantee. modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/status/GetTieringStatusAction.java [23-25] /** Constructs a new GetTieringStatusAction. */ -public GetTieringStatusAction() { +private GetTieringStatusAction() { super(NAME, GetTieringStatusResponse::new); } Suggestion importance[1-10]: 7 __ Why: The public constructor on GetTieringStatusAction breaks the singleton pattern enforced by INSTANCE, unlike all other similar action classes (ListTieringStatusAction, CancelTieringAction, etc.) which use private constructors. This is a real inconsistency that could lead to unintended multiple instances.	Medium
	Add null validation before dereferencing builder fields The constructor accesses builder.directory without a null check, which will throw a NullPointerException if directory is not set on the builder before calling build(). Since blockFileName could also be null, resolve(blockFileName) would also fail. Add null validation in the build() method or constructor before dereferencing these fields. modules/tiered-storage/src/main/java/org/opensearch/storage/indexinput/BlockFetchRequest.java [31-38] private BlockFetchRequest(Builder builder) { + if (builder.directory == null) { + throw new IllegalStateException("directory must be set"); + } + if (builder.blockFileName == null) { + throw new IllegalStateException("blockFileName must be set"); + } this.fileName = builder.fileName; this.blockFileName = builder.blockFileName; this.filePath = builder.directory.getDirectory().resolve(blockFileName); this.directory = builder.directory; this.blockSize = builder.blockSize; this.blockStart = builder.blockStart; } Suggestion importance[1-10]: 6 __ Why: The constructor dereferences builder.directory and builder.blockFileName without null checks, which could cause NullPointerException if the builder is used without setting these required fields. This is a valid defensive programming concern, though the builder pattern typically relies on callers to set required fields.	Low
	Avoid partial stream consumption before throwing in StreamInput constructors The StreamInput constructor calls super(in) which reads from the stream, and then immediately throws UnsupportedOperationException. This means any attempt to deserialize this request (e.g., during transport) will partially consume the stream and then fail with an unchecked exception, potentially corrupting the stream state. The same pattern exists in GetTieringStatusRequest, ListTieringStatusRequest, and IndexTieringRequest. Consider not calling super(in) before throwing, or document clearly that deserialization is not yet supported. [modules/tiered-storage/src/main/java/org/opensearch/storage/action/tiering/CancelTieringRequest.java [39-42]](https://github.com/opensea...

[github-actions]

Persistent review updated to latest commit 130e125

[github-actions]

✅ Gradle check result for 130e125: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 0% with 510 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.00%. Comparing base (04ac6e3) to head (5133192).

Files with missing lines	Patch %	Lines
...age/action/tiering/status/model/TieringStatus.java	0.00%	48 Missing ⚠️
...h/storage/slowlogs/TieredStorageSearchSlowLog.java	0.00%	43 Missing ⚠️
...earch/storage/indexinput/SwitchableIndexInput.java	0.00%	38 Missing ⚠️
...rage/slowlogs/TieredStorageQueryMetricService.java	0.00%	35 Missing ⚠️
...ensearch/storage/indexinput/BlockFetchRequest.java	0.00%	26 Missing ⚠️
.../tiering/status/model/GetTieringStatusRequest.java	0.00%	19 Missing ⚠️
...pensearch/storage/common/tiering/TieringUtils.java	0.00%	18 Missing ⚠️
...rage/common/tiering/TieringRejectionException.java	0.00%	17 Missing ⚠️
...org/opensearch/storage/slowlogs/PrefetchStats.java	0.00%	15 Missing ⚠️
...org/opensearch/storage/tiering/TieringService.java	0.00%	14 Missing ⚠️
... and 40 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21017      +/-   ##
============================================
- Coverage     73.24%   73.00%   -0.25%     
+ Complexity    72811    72749      -62     
============================================
  Files          5871     5921      +50     
  Lines        332666   333176     +510     
  Branches      48014    48017       +3     
============================================
- Hits         243660   243230     -430     
- Misses        69451    70460    +1009     
+ Partials      19555    19486      -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[chaitanya588]

In few classes, i am seeing default constructors. Can you add the specific overloaded constructors here?

1. TransportTierAction
2. TransportGetTieringStatusAction
3. TransportListTieringStatusAction
4. TransportCancelTierAction

[github-actions]

Persistent review updated to latest commit e96ceb8

[github-actions]

❌ Gradle check result for e96ceb8: 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 a30ef73

[github-actions]

✅ Gradle check result for a30ef73: SUCCESS

[github-actions]

Persistent review updated to latest commit a81330c

[github-actions]

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

Failed to generate code suggestions for PR

[github-actions]

❌ Gradle check result for ed51490: null

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 3dbfec7

[github-actions]

Persistent review updated to latest commit 5cd1173

[github-actions]

❌ Gradle check result for 5cd1173: 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 5133192

[github-actions]

✅ Gradle check result for 5133192: SUCCESS