server/src/main/java/org/opensearch/gateway/remote/RemoteStateCleanupFailedException.java (1)

22-25: Redundant toString() override.

This override simply delegates to super.toString(), which is the default behavior. It can be removed.

♻️ Suggested removal

 public class RemoteStateCleanupFailedException extends RuntimeException {

     public RemoteStateCleanupFailedException(ParameterizedMessage parameterizedMessage, Throwable cause) {
         super(parameterizedMessage.toString(), cause);
     }
-
-    `@Override`
-    public String toString() {
-        return super.toString();
-    }
 }

plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerTimeoutTests.java (1)

39-95: Consider adding edge-case tests for timeout behavior.

The current tests cover the happy path (successful completion) and verify call delegation, which is good. Consider adding tests for:

• TimeValue.MINUS_ONE → verifies future.get() (infinite wait) path
• A future that doesn't complete within the timeout → verifies TimeoutException handling and future cancellation

These would exercise the branching logic in getFutureValue more thoroughly.

server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java (1)

285-295: Javadoc is missing @param timeout documentation.

The new timeout parameter is not documented in the Javadoc. Also, consider whether warn level is appropriate here — if a concrete implementation never overrides this method, every cleanup call will emit a warning. debug or info might be less noisy for a fallback path that's expected for most BlobContainer implementations.

Proposed Javadoc and log-level fix

     /**
      * Deletes the blobs with given names. This method will not throw an exception
      * when one or multiple of the given blobs don't exist and simply ignore this case.
      *
      * `@param`   blobNames  The names of the blob to delete.
+     * `@param`   timeout    The timeout for the delete operation. Implementations that do not
+     *                     support timeouts will ignore this parameter and delegate to
+     *                     {`@link` `#deleteBlobsIgnoringIfNotExists`(List)}.
      * `@throws`  IOException if a subset of blob exists but could not be deleted.
      */
     default void deleteBlobsIgnoringIfNotExists(List<String> blobNames, TimeValue timeout) throws IOException {
-        LogManager.getLogger(this.getClass()).warn("No implementation exists for using timeout, skipping timeout input");
+        LogManager.getLogger(this.getClass()).debug("No implementation exists for using timeout, skipping timeout input");
         deleteBlobsIgnoringIfNotExists(blobNames);
     }

server/src/internalClusterTest/java/org/opensearch/gateway/remote/RemoteClusterStateCleanupManagerIT.java (1)

324-331: Prefer static import over inline fully-qualified class reference.

java.util.stream.Collectors is used inline on lines 330 and 347 as java.util.stream.Collectors.toList(). Consider adding a static import for consistency with the rest of the file's import style.

Add import at top of file

+import java.util.stream.Collectors;

Then replace java.util.stream.Collectors.toList() with Collectors.toList() on lines 330 and 347.

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java (1)

80-86: Consider delegating to the non-timeout variant instead of throwing.

The BlobContainer interface handles the same pattern by falling back to the non-timeout overload in its default method. Here, the default throws UnsupportedOperationException, which means any RemoteRoutingTableService implementation that doesn't override these will fail at runtime rather than degrade gracefully.

Since InternalRemoteRoutingTableService does override these, this isn't an immediate issue, but a consistent fallback pattern would be more defensive.

Proposed fallback to non-timeout variant

     default void deleteStaleIndexRoutingPaths(List<String> stalePaths, TimeValue timeout) throws IOException {
-        throw new UnsupportedOperationException("deleteStaleIndexRoutingPaths with timeouts is not supported");
+        deleteStaleIndexRoutingPaths(stalePaths);
     }

     default void deleteStaleIndexRoutingDiffPaths(List<String> stalePaths, TimeValue timeout) throws IOException {
-        throw new UnsupportedOperationException("deleteStaleIndexRoutingDiffPaths with timeouts is not supported");
+        deleteStaleIndexRoutingDiffPaths(stalePaths);
     }

server/src/main/java/org/opensearch/cluster/routing/remote/InternalRemoteRoutingTableService.java (1)

271-311: Consider reducing duplication between timeout and non-timeout variants.

The four methods (lines 271–311) share near-identical structure — the only difference is the extra TimeValue parameter. You could have the non-timeout methods delegate to the timeout variants with a null timeout, reducing the duplicated try/catch/log/rethrow blocks.

Example: delegate non-timeout to timeout variant

     `@Override`
     public void deleteStaleIndexRoutingPaths(List<String> stalePaths) throws IOException {
-        try {
-            logger.debug(() -> "Deleting stale index routing files from remote - " + stalePaths);
-            blobStoreRepository.blobStore().blobContainer(BlobPath.cleanPath()).deleteBlobsIgnoringIfNotExists(stalePaths);
-        } catch (IOException e) {
-            logger.error(() -> new ParameterizedMessage("Failed to delete some stale index routing paths from {}", stalePaths), e);
-            throw e;
-        }
+        deleteStaleIndexRoutingPaths(stalePaths, null);
     }

     `@Override`
     public void deleteStaleIndexRoutingPaths(List<String> stalePaths, TimeValue timeValue) throws IOException {
         try {
             logger.debug(() -> "Deleting stale index routing files from remote - " + stalePaths);
-            blobStoreRepository.blobStore().blobContainer(BlobPath.cleanPath()).deleteBlobsIgnoringIfNotExists(stalePaths, timeValue);
+            if (timeValue != null) {
+                blobStoreRepository.blobStore().blobContainer(BlobPath.cleanPath()).deleteBlobsIgnoringIfNotExists(stalePaths, timeValue);
+            } else {
+                blobStoreRepository.blobStore().blobContainer(BlobPath.cleanPath()).deleteBlobsIgnoringIfNotExists(stalePaths);
+            }
         } catch (IOException e) {
             logger.error(() -> new ParameterizedMessage("Failed to delete some stale index routing paths from {}", stalePaths), e);
             throw e;
         }
     }

Same pattern for the diff-path methods.

server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateCleanupManagerTests.java (1)

908-1013: Good batch-processing test coverage.

The four batched-deletion tests (testBatchedDeletionWithSingleBatch, testBatchedDeletionWithMultipleBatches, testBatchedDeletionStopsWhenManifestsLessThanRetained, testBatchedDeletionExhaustsMaxBatches) comprehensively cover the key scenarios. However, there is no test for the failure path where deleteClusterMetadata throws mid-batch and RemoteStateCleanupFailedException is raised. Adding such a test would help verify batch-level error handling and, importantly, would surface the lastCleanupAttemptStateVersion-advancement issue described in the production code review.

server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateCleanupManager.java (2)

179-201: Rescheduling the async deletion task on non-interval setting changes is unnecessary.

rescheduleAsyncDeletionTask() is called when cleanupBatchSize, cleanupMaxBatches, or cleanupTimeout are updated. These don't affect the task's scheduling interval — the next run will simply pick up the new field values. Only updateCleanupInterval genuinely needs to reschedule. The rescheduleIfNecessary() call is harmless but adds noise.

---

111-113: Consider volatile for fields written on the cluster-applier thread and read on REMOTE_PURGE.

cleanupBatchSize, cleanupMaxBatches, and cleanupTimeout are written by settings-update consumers (cluster-applier thread) and read by the batch-processing loop (REMOTE_PURGE thread). Without volatile, updated values may not be visible to the reader thread due to the Java Memory Model. This is consistent with the existing non-volatile staleFileCleanupInterval, but the new fields are read in a tight loop during batch processing, increasing the window for stale reads.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20411
File: server/src/main/java/org/opensearch/index/codec/CodecService.java:112-133
Timestamp: 2026-01-13T17:40:27.167Z
Learning: Avoid capturing or evaluating a supplier (e.g., this::defaultCodec) upfront when passing it to a registry during object construction. If registries may replace defaults during iteration (as in EnginePlugin.getAdditionalCodecs), pass the supplier itself and only resolve it at use time. This ensures dynamic behavior is preserved during initialization and prevents premature binding of defaults in codecs/registry setup. This pattern should apply to similar initialization paths in Java server code where registries may mutate defaults during construction.

Learnt from: udabhas
Repo: opensearch-project/OpenSearch PR: 20095
File: server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java:4146-4165
Timestamp: 2026-01-14T09:31:12.773Z
Learning: For OpenSearch’s S3 path, index-level CryptoMetadata is passed from BlobStoreRepository to S3BlobContainer, and the final KMS key/context merge with repository settings is performed in SseKmsUtil.configureEncryptionSettings. Do not merge in BlobStoreRepository to avoid double-merging.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}