DataFormat-aware shallow snapshot v2 support

[ask-kamal-nayan]

Description

Description

Wires DataFormat-Aware (DFA) indices into the existing shallow snapshot v2
path so V2 (pinned-timestamp) snapshots and restores work end-to-end for all the supported data formats.

Builds on #21311 (DFA NRT replication engine and remote-store wiring) by
making the cleanup path format-aware: when the live IndexService is gone but
the snapshotted IndexMetadata is available, cleanup now routes DFA indices
through DataFormatAwareRemoteDirectory so all per-format files are
enumerated, rather than leaking parquet/segments dirs.

What changes

• New IndexMetadata-aware overloads on
  RemoteSegmentStoreDirectoryFactory.newDirectory(...) and
  RemoteSegmentStoreDirectory.remoteDirectoryCleanup(...). Existing
  overloads are preserved and delegate to the new variants with null.
• Call sites in BlobStoreRepository, SnapshotsService,
  TransportCleanupRepositoryAction, IndexShard, StoreRecovery, and
  Node updated to thread IndexMetadata through.
• New IT class DataFormatAwareRestoreShallowSnapshotV2IT (18 tests)
  covering V2 snapshot create / restore / clone / delete / multi-shard /
  concurrent / rename / pinned-timestamp cleanup / catalog-generation
  preservation on DFA indices, plus parameterized variant
  DataFormatAwareRestoreShallowSnapshotV2WithLuceneIT exercising the
  parquet + lucene-secondary code paths.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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 Reviewer Guide 🔍

(Review updated until commit 04e8764)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The UUID assertion logic is inverted. Line 965 asserts assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid), but the comment on line 956-958 states "restore always creates a fresh index with a NEW UUID (not the snapshot's source UUID)". However, the test captures pre.indexUUID from the source index before snapshot (line 882), then after restore it fetches the restored index's UUID (line 959-963). The assertion should confirm they differ, which it does—but the parameter requireSameUUID (line 918) is misleading: when true, the method still asserts UUIDs differ (line 965). This contradicts the parameter name and could cause confusion or incorrect usage in subclasses. // For rename, the new UUID is also different. So in both cases the restored UUID must // (a) be set, (b) differ from the pre-snapshot source UUID. String restoredUuid = client.admin() .indices() .prepareGetSettings(restoredIndexName) .get() .getSetting(restoredIndexName, IndexMetadata.SETTING_INDEX_UUID); assertNotNull("restored index UUID must be set", restoredUuid); assertFalse("restored index UUID must be non-empty", restoredUuid.isEmpty()); assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); Possible Issue The countFilesUnder method (lines 1556-1587) uses a mutable array long[] count = { 0 } to accumulate file counts inside a FileVisitor. If Files.walkFileTree is interrupted or throws an exception mid-traversal, the count may be incomplete, but the method returns it without signaling the error. The visitFileFailed override (line 1580) silently continues on IOException, which is correct for concurrent cleanup tolerance, but if the root path itself is inaccessible or a critical I/O error occurs during traversal setup, the method returns 0 without distinguishing "no files found" from "traversal failed". This could cause testV2DeleteSnapshotCleansUpAllFormatFilesForDFA (line 1483) to pass incorrectly if cleanup verification fails due to I/O errors rather than actual cleanup success. private static long countFilesUnder(Path rootPath, String indexUUID, String categoryDirName) throws IOException { if (Files.exists(rootPath) == false) return 0; long[] count = { 0 }; Files.walkFileTree(rootPath, new java.nio.file.SimpleFileVisitor<>() { @Override public java.nio.file.FileVisitResult visitFile(Path file, java.nio.file.attribute.BasicFileAttributes attrs) { // Iterate path name elements rather than substring-matching the toString() — the latter // breaks on Windows where the separator is '\\', and also matches non-component substrings. boolean hasUuid = false; boolean hasCategory = false; for (Path part : file) { String name = part.toString(); if (indexUUID.equals(name)) { hasUuid = true; } else if (categoryDirName.equals(name)) { hasCategory = true; } } if (hasUuid && hasCategory) { count[0]++; } return java.nio.file.FileVisitResult.CONTINUE; } @Override public java.nio.file.FileVisitResult visitFileFailed(Path file, IOException exc) { // Concurrent cleanup may delete a file between enumeration and visit; tolerate. return java.nio.file.FileVisitResult.CONTINUE; } }); return count[0]; } Possible Issue The deprecated 6-arg remoteDirectoryCleanup overload (line 1345) delegates to the 7-arg variant with null IndexMetadata (line 1353). The comment (line 1339-1342) warns this "may leak [per-format files] on cleanup of DFA indices". However, the method is marked @Deprecated without a removal timeline, and existing callers (e.g., BlobStoreRepository line 1715 in the diff) still use it for V1 snapshots. If a DFA index is ever associated with a V1 snapshot (despite the comment claiming "DFA indices use V2 snapshots only"), the cleanup will silently leak parquet/ files. The code should either enforce that DFA indices cannot use V1 snapshots (via a runtime check) or log a warning when the deprecated overload is called with a DFA-enabled index UUID. /** * Backward-compatible 6-arg overload preserved for the 2.3.0 {@code @PublicApi} contract. * Delegates to the 7-arg variant with a {@code null} {@link IndexMetadata} — equivalent to * the prior behaviour for callers that don't need data-format-aware routing. * * @deprecated Use the 7-arg variant that accepts {@link IndexMetadata} so that DFA-enabled * indices route to {@link org.opensearch.index.store.remote.DataFormatAwareRemoteDirectory} * during cleanup. This overload remains for backward compatibility but does not * enumerate per-format files (e.g., {@code parquet/}) and may leak them on cleanup * of DFA indices. */ @Deprecated public static void remoteDirectoryCleanup( RemoteSegmentStoreDirectoryFactory remoteDirectoryFactory, String remoteStoreRepoForIndex, String indexUUID, ShardId shardId, RemoteStorePathStrategy pathStrategy, boolean forceClean ) { remoteDirectoryCleanup(remoteDirectoryFactory, remoteStoreRepoForIndex, indexUUID, shardId, pathStrategy, forceClean, null); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 04e8764

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Restore global settings after test Modifying global RemoteStoreSettings via static setter in a test can cause unpredictable side effects in concurrent or subsequent tests. The lookback interval change persists beyond this test's scope. Restore the original value in a finally block or use test-scoped settings to prevent cross-test pollution. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [1524-1532] public void testV2DeleteSnapshotCleansUpAllFormatFilesForDFA() throws Exception { ... - // Configure aggressive pinned-timestamp release so cleanup can fire promptly String clusterManagerName = internalCluster().getClusterManagerName(); RemoteStorePinnedTimestampService remoteStorePinnedTimestampService = internalCluster().getInstance( RemoteStorePinnedTimestampService.class, clusterManagerName ); - RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO); - remoteStorePinnedTimestampService.rescheduleAsyncUpdatePinnedTimestampTask(TimeValue.timeValueSeconds(1)); - keepPinnedTimestampSchedulerUpdated(); + TimeValue originalLookback = RemoteStoreSettings.getPinnedTimestampsLookbackInterval(); + try { + RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO); + remoteStorePinnedTimestampService.rescheduleAsyncUpdatePinnedTimestampTask(TimeValue.timeValueSeconds(1)); + keepPinnedTimestampSchedulerUpdated(); + ... + } finally { + RemoteStoreSettings.setPinnedTimestampsLookbackInterval(originalLookback); + } Suggestion importance[1-10]: 7 __ Why: This is a valid concern about test isolation. Modifying global RemoteStoreSettings via static setter can cause side effects in concurrent or subsequent tests. The suggestion to restore the original value in a finally block is a good practice for test hygiene, though the actual impact depends on test execution order and parallelization.	Medium
	Log warning for null metadata The deprecated 6-arg overload delegates to the 7-arg variant with null for IndexMetadata, which prevents DFA-enabled indices from routing to DataFormatAwareRemoteDirectory during cleanup. This can leak per-format files (e.g., parquet/). Add a warning log when indexMetadata is null in the 7-arg method to alert operators of potential cleanup issues. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java [1356-1380] -@Deprecated public static void remoteDirectoryCleanup( RemoteSegmentStoreDirectoryFactory remoteDirectoryFactory, String remoteStoreRepoForIndex, String indexUUID, ShardId shardId, RemoteStorePathStrategy pathStrategy, - boolean forceClean + boolean forceClean, + IndexMetadata indexMetadata ) { - remoteDirectoryCleanup(remoteDirectoryFactory, remoteStoreRepoForIndex, indexUUID, shardId, pathStrategy, forceClean, null); -} + try { + if (indexMetadata == null) { + logger.warn("remoteDirectoryCleanup called with null IndexMetadata for indexUUID={}; DFA per-format files may not be cleaned", indexUUID); + } + IndexSettings indexSettings = indexMetadata != null ? new IndexSettings(indexMetadata, Settings.EMPTY) : null; + ... Suggestion importance[1-10]: 5 __ Why: Adding a warning log when indexMetadata is null is a reasonable defensive measure to alert operators of potential cleanup issues with DFA indices. However, the suggestion's impact is moderate because the deprecation notice already documents this limitation, and the warning would only help at runtime. The score reflects that this is a helpful improvement but not critical.	Low
	Short-circuit path iteration early The visitFile callback increments count[0] for every file matching both indexUUID and categoryDirName in its path. If the directory tree is deep or contains many files, this can become a performance bottleneck. Consider short-circuiting the path iteration once both conditions are met to reduce unnecessary string comparisons. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [1560-1577] -private static long countFilesUnder(Path rootPath, String indexUUID, String categoryDirName) throws IOException { - if (Files.exists(rootPath) == false) return 0; - long[] count = { 0 }; - Files.walkFileTree(rootPath, new java.nio.file.SimpleFileVisitor<>() { - @Override - public java.nio.file.FileVisitResult visitFile(Path file, java.nio.file.attribute.BasicFileAttributes attrs) { - boolean hasUuid = false; - boolean hasCategory = false; - for (Path part : file) { - String name = part.toString(); - if (indexUUID.equals(name)) { - hasUuid = true; - } else if (categoryDirName.equals(name)) { - hasCategory = true; - } - } - if (hasUuid && hasCategory) { - count[0]++; - } +@Override +public java.nio.file.FileVisitResult visitFile(Path file, java.nio.file.attribute.BasicFileAttributes attrs) { + boolean hasUuid = false; + boolean hasCategory = false; + for (Path part : file) { + String name = part.toString(); + if (indexUUID.equals(name)) { + hasUuid = true; + } else if (categoryDirName.equals(name)) { + hasCategory = true; + } + if (hasUuid && hasCategory) { + count[0]++; return java.nio.file.FileVisitResult.CONTINUE; } + } + return java.nio.file.FileVisitResult.CONTINUE; +} Suggestion importance[1-10]: 4 __ Why: The suggestion to short-circuit once both conditions are met is a minor optimization. However, the improved code has a logic error: it increments count[0] and returns immediately when both conditions are met, but this happens inside the loop over path parts, meaning it would return after finding the first matching part rather than after checking all parts. The optimization is valid in principle but the implementation is incorrect.	Low
	Remove unused boolean parameter The requireSameUUID parameter is passed to the method but never used in the UUID validation logic. The comment states "restore always creates a fresh index with a NEW UUID", yet the parameter name suggests conditional behavior. Either remove the unused parameter or implement the conditional logic it implies to avoid misleading future maintainers. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [913-971] protected void assertRestoredIndexMatches( Client client, String restoredIndexName, IndexShard restoredShard, - PreSnapshotState pre, - boolean requireSameUUID + PreSnapshotState pre ) throws IOException { ... // UUID check: restore always creates a fresh index with a NEW UUID (not the snapshot's source UUID). - // For rename, the new UUID is also different. So in both cases the restored UUID must - // (a) be set, (b) differ from the pre-snapshot source UUID. String restoredUuid = client.admin() .indices() .prepareGetSettings(restoredIndexName) .get() .getSetting(restoredIndexName, IndexMetadata.SETTING_INDEX_UUID); assertNotNull("restored index UUID must be set", restoredUuid); assertFalse("restored index UUID must be non-empty", restoredUuid.isEmpty()); assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); Suggestion importance[1-10]: 3 __ Why: The requireSameUUID parameter is indeed unused in the method body. However, the suggestion's impact is limited because the parameter was intentionally added for future extensibility (as indicated by the comment about rename tests). Removing it would require updating all call sites and might reduce code clarity for future maintainers who need to understand the UUID behavior differences.	Low

---

Previous suggestions

Suggestions up to commit 04e8764

Category	Suggestion	Impact
General	Global settings modified without cleanup The test modifies global cluster settings (RemoteStoreSettings.setPinnedTimestampsLookbackInterval) without restoring them after the test completes. This can cause test pollution where subsequent tests inherit the modified lookback interval, leading to flaky failures. Capture the original value and restore it in a finally block or use @After cleanup. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [1524-1532] public void testV2DeleteSnapshotCleansUpAllFormatFilesForDFA() throws Exception { ... - // Configure aggressive pinned-timestamp release so cleanup can fire promptly String clusterManagerName = internalCluster().getClusterManagerName(); RemoteStorePinnedTimestampService remoteStorePinnedTimestampService = internalCluster().getInstance( RemoteStorePinnedTimestampService.class, clusterManagerName ); - RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO); - remoteStorePinnedTimestampService.rescheduleAsyncUpdatePinnedTimestampTask(TimeValue.timeValueSeconds(1)); - keepPinnedTimestampSchedulerUpdated(); + TimeValue originalLookback = RemoteStoreSettings.getPinnedTimestampsLookbackInterval(); + try { + RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO); + remoteStorePinnedTimestampService.rescheduleAsyncUpdatePinnedTimestampTask(TimeValue.timeValueSeconds(1)); + keepPinnedTimestampSchedulerUpdated(); + ... + } finally { + RemoteStoreSettings.setPinnedTimestampsLookbackInterval(originalLookback); + } Suggestion importance[1-10]: 8 __ Why: The test modifies global cluster settings (RemoteStoreSettings.setPinnedTimestampsLookbackInterval) without restoring them after the test completes. This can cause test pollution where subsequent tests inherit the modified lookback interval, leading to flaky failures. This is a significant test hygiene issue that should be addressed.	Medium
	Unused parameter in validation method The requireSameUUID parameter is passed but never used in the UUID validation logic. The method always asserts that the restored UUID differs from the source UUID, contradicting the parameter's intent. Either remove the unused parameter or implement conditional logic that checks requireSameUUID to decide whether to assert equality or inequality. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [913-971] protected void assertRestoredIndexMatches( Client client, String restoredIndexName, IndexShard restoredShard, PreSnapshotState pre, boolean requireSameUUID ) throws IOException { ... - // UUID check: restore always creates a fresh index with a NEW UUID (not the snapshot's source UUID). - // For rename, the new UUID is also different. So in both cases the restored UUID must - // (a) be set, (b) differ from the pre-snapshot source UUID. String restoredUuid = client.admin() .indices() .prepareGetSettings(restoredIndexName) .get() .getSetting(restoredIndexName, IndexMetadata.SETTING_INDEX_UUID); assertNotNull("restored index UUID must be set", restoredUuid); assertFalse("restored index UUID must be non-empty", restoredUuid.isEmpty()); - assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); + if (requireSameUUID) { + assertEquals("restored index UUID must match source UUID when requireSameUUID=true", pre.indexUUID, restoredUuid); + } else { + assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); + } Suggestion importance[1-10]: 7 __ Why: The requireSameUUID parameter is passed but never used in the UUID validation logic. The method always asserts that the restored UUID differs from the source UUID, contradicting the parameter's intent. However, the comment at line 955-957 explicitly states "restore always creates a fresh index with a NEW UUID", which suggests the current behavior may be intentional. The parameter should either be removed or the logic should be implemented to respect it.	Medium
	Deprecation notice lacks critical warning The deprecated 6-arg overload delegates to the 7-arg variant with null for IndexMetadata, which prevents DFA-enabled indices from routing to DataFormatAwareRemoteDirectory during cleanup. This can leak per-format files (e.g., parquet/) on cleanup. The deprecation notice should explicitly warn callers that DFA indices will not be cleaned correctly and recommend immediate migration to the 7-arg variant. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java [1333-1354] +/** + * @deprecated Use the 7-arg variant that accepts {@link IndexMetadata}. This overload passes + * {@code null} for IndexMetadata, which prevents DFA-enabled indices from routing + * to {@link org.opensearch.index.store.remote.DataFormatAwareRemoteDirectory}. + * <strong>WARNING:</strong> Calling this method on DFA indices will leak per-format + * files (e.g., {@code parquet/}) during cleanup. Migrate to the 7-arg variant immediately. + */ @Deprecated public static void remoteDirectoryCleanup( RemoteSegmentStoreDirectoryFactory remoteDirectoryFactory, String remoteStoreRepoForIndex, String indexUUID, ShardId shardId, RemoteStorePathStrategy pathStrategy, boolean forceClean ) { remoteDirectoryCleanup(remoteDirectoryFactory, remoteStoreRepoForIndex, indexUUID, shardId, pathStrategy, forceClean, null); } Suggestion importance[1-10]: 6 __ Why: The deprecated 6-arg overload delegates to the 7-arg variant with null for IndexMetadata, which prevents DFA-enabled indices from routing to DataFormatAwareRemoteDirectory during cleanup. While the deprecation notice mentions this limitation, it could be more explicit about the potential for file leaks on DFA indices. The suggestion to strengthen the warning is valid but has moderate impact since it's already deprecated.	Low
	Unused helper method provides no coverage The method assertCatalogFilesRestoredOnDisk is defined but never called in any test. This indicates dead code that provides no test coverage. Either remove the method if it's obsolete, or add a test case that invokes it to validate catalog file restoration on disk. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [985-1005] -private void assertCatalogFilesRestoredOnDisk(IndexShard shardBefore, IndexShard shardAfter) throws IOException { - Set<String> catalogBefore = DataFormatAwareITUtils.catalogFilesExcludingSegments(shardBefore); - Set<String> filesAfter = captureShardFilesOnDisk(shardAfter); - assertFalse("catalog before snapshot must not be empty for shard " + shardBefore.routingEntry(), catalogBefore.isEmpty()); - Set<String> missing = new HashSet<>(); - for (String catalogFile : catalogBefore) { - // catalog file names may already be format-prefixed (e.g. "parquet/foo.parquet") - if (filesAfter.contains(catalogFile) == false && filesAfter.contains("index/" + catalogFile) == false) { - missing.add(catalogFile); - } - } - assertTrue( - "catalog files missing from restored shard disk: " - + missing - + "\n catalog before: " - + catalogBefore - + "\n files on disk after restore: " - + filesAfter, - missing.isEmpty() - ); -} +// Remove this method if it's not needed, or add a test that calls it: +// Example usage in testV2SnapshotCreateAndRestoreForDFAIndex: +// assertCatalogFilesRestoredOnDisk(shardBefore, shardAfter); Suggestion importance[1-10]: 4 __ Why: The method assertCatalogFilesRestoredOnDisk is defined but never called in any test. This indicates dead code that provides no test coverage. While removing dead code improves maintainability, the impact is relatively low since the method doesn't affect runtime behavior or correctness.	Low

Suggestions up to commit 92d2060

Category	Suggestion	Impact
General	Implement unused requireSameUUID parameter logic The requireSameUUID parameter is never used in the method body, yet the method's logic unconditionally asserts that the restored UUID differs from the pre-snapshot UUID. This contradicts the parameter's name and the method's signature, which suggests conditional UUID validation. Either remove the unused parameter or implement the conditional logic it implies. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [913-971] protected void assertRestoredIndexMatches( Client client, String restoredIndexName, IndexShard restoredShard, PreSnapshotState pre, boolean requireSameUUID ) throws IOException { ... - // UUID check: restore always creates a fresh index with a NEW UUID (not the snapshot's source UUID). - // For rename, the new UUID is also different. So in both cases the restored UUID must - // (a) be set, (b) differ from the pre-snapshot source UUID. String restoredUuid = client.admin() .indices() .prepareGetSettings(restoredIndexName) .get() .getSetting(restoredIndexName, IndexMetadata.SETTING_INDEX_UUID); assertNotNull("restored index UUID must be set", restoredUuid); assertFalse("restored index UUID must be non-empty", restoredUuid.isEmpty()); - assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); + if (requireSameUUID) { + assertEquals("restored index UUID must match pre-snapshot UUID when requireSameUUID=true", pre.indexUUID, restoredUuid); + } else { + assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); + } ... } Suggestion importance[1-10]: 7 __ Why: The requireSameUUID parameter is declared but never used in the method body. The method unconditionally asserts that the restored UUID differs from the pre-snapshot UUID, which contradicts the parameter's intent. However, the comment at line 955-957 explicitly states that "restore always creates a fresh index with a NEW UUID", suggesting the current behavior is intentional. The parameter appears to be a design artifact that should either be removed or implemented.	Medium
	Warn on deprecated cleanup method usage The deprecated 6-arg remoteDirectoryCleanup overload passes null for IndexMetadata, which causes DFA indices to skip per-format file cleanup (e.g., parquet/ directories). Callers using this deprecated method on DFA indices will leak files. Add a runtime warning or assertion to detect DFA usage and guide migration to the 7-arg variant. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java [1344-1354] @Deprecated public static void remoteDirectoryCleanup( RemoteSegmentStoreDirectoryFactory remoteDirectoryFactory, String remoteStoreRepoForIndex, String indexUUID, ShardId shardId, RemoteStorePathStrategy pathStrategy, boolean forceClean ) { + logger.warn("Deprecated 6-arg remoteDirectoryCleanup called for indexUUID={}. DFA indices may leak per-format files. Migrate to 7-arg variant.", indexUUID); remoteDirectoryCleanup(remoteDirectoryFactory, remoteStoreRepoForIndex, indexUUID, shardId, pathStrategy, forceClean, null); } Suggestion importance[1-10]: 6 __ Why: The deprecated 6-arg overload passes null for IndexMetadata, which may cause DFA indices to skip per-format file cleanup. Adding a warning would help detect misuse and guide migration. However, the deprecation comment already documents this limitation, and the method is marked @Deprecated with clear guidance to use the 7-arg variant.	Low
	Remove or invoke unused validation method The assertCatalogFilesRestoredOnDisk method is defined but never invoked anywhere in the test class. This dead code should be removed or integrated into the test validation flow to prevent maintenance burden and confusion about its purpose. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [985-1005] -private void assertCatalogFilesRestoredOnDisk(IndexShard shardBefore, IndexShard shardAfter) throws IOException { - Set<String> catalogBefore = DataFormatAwareITUtils.catalogFilesExcludingSegments(shardBefore); - Set<String> filesAfter = captureShardFilesOnDisk(shardAfter); - assertFalse("catalog before snapshot must not be empty for shard " + shardBefore.routingEntry(), catalogBefore.isEmpty()); - Set<String> missing = new HashSet<>(); - for (String catalogFile : catalogBefore) { - // catalog file names may already be format-prefixed (e.g. "parquet/foo.parquet") - if (filesAfter.contains(catalogFile) == false && filesAfter.contains("index/" + catalogFile) == false) { - missing.add(catalogFile); - } - } - assertTrue( - "catalog files missing from restored shard disk: " - + missing - + "\n catalog before: " - + catalogBefore - + "\n files on disk after restore: " - + filesAfter, - missing.isEmpty() - ); -} +// Remove the entire assertCatalogFilesRestoredOnDisk method if it's not needed, +// or add a call to it in relevant test methods like testV2SnapshotCreateAndRestoreForDFAIndex: +assertCatalogFilesRestoredOnDisk(shardBefore, shardAfter); Suggestion importance[1-10]: 5 __ Why: The assertCatalogFilesRestoredOnDisk method is defined but never called in the test class. While this is dead code that could be removed, the method appears to provide useful validation logic that could strengthen the tests if integrated. The impact is moderate since it doesn't affect correctness, only test coverage.	Low

Suggestions up to commit 92d2060

Category	Suggestion	Impact
General	Restore modified cluster settings The test modifies global cluster settings (setPinnedTimestampsLookbackInterval) without restoring them afterward. This can cause test pollution where subsequent tests inherit the modified settings. Store the original value before modification and restore it in a finally block or @After method to ensure test isolation. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [1526-1532] public void testV2DeleteSnapshotCleansUpAllFormatFilesForDFA() throws Exception { ... - // Configure aggressive pinned-timestamp release so cleanup can fire promptly String clusterManagerName = internalCluster().getClusterManagerName(); RemoteStorePinnedTimestampService remoteStorePinnedTimestampService = internalCluster().getInstance( RemoteStorePinnedTimestampService.class, clusterManagerName ); - RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO); - remoteStorePinnedTimestampService.rescheduleAsyncUpdatePinnedTimestampTask(TimeValue.timeValueSeconds(1)); + TimeValue originalLookback = RemoteStoreSettings.getPinnedTimestampsLookbackInterval(); + try { + RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO); + remoteStorePinnedTimestampService.rescheduleAsyncUpdatePinnedTimestampTask(TimeValue.timeValueSeconds(1)); + ... + } finally { + RemoteStoreSettings.setPinnedTimestampsLookbackInterval(originalLookback); + } Suggestion importance[1-10]: 8 __ Why: The test modifies global cluster settings via RemoteStoreSettings.setPinnedTimestampsLookbackInterval(TimeValue.ZERO) without restoring the original value. This can cause test pollution where subsequent tests inherit the modified settings, leading to flaky or incorrect test behavior. Storing and restoring the original value in a try-finally block ensures proper test isolation and prevents side effects.	Medium
	Implement unused parameter logic The requireSameUUID parameter is passed but never used in the UUID validation logic. The assertion always expects a different UUID regardless of the parameter value. Either remove the unused parameter or implement conditional logic that checks requireSameUUID to determine whether to assert equality or inequality of UUIDs. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [913-971] protected void assertRestoredIndexMatches( Client client, String restoredIndexName, IndexShard restoredShard, PreSnapshotState pre, boolean requireSameUUID ) throws IOException { ... - // UUID check: restore always creates a fresh index with a NEW UUID (not the snapshot's source UUID). - // For rename, the new UUID is also different. So in both cases the restored UUID must - // (a) be set, (b) differ from the pre-snapshot source UUID. String restoredUuid = client.admin() .indices() .prepareGetSettings(restoredIndexName) .get() .getSetting(restoredIndexName, IndexMetadata.SETTING_INDEX_UUID); assertNotNull("restored index UUID must be set", restoredUuid); assertFalse("restored index UUID must be non-empty", restoredUuid.isEmpty()); - assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); + if (requireSameUUID) { + assertEquals("restored index UUID must match source UUID when requireSameUUID=true", pre.indexUUID, restoredUuid); + } else { + assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); + } Suggestion importance[1-10]: 7 __ Why: The requireSameUUID parameter is passed to assertRestoredIndexMatches but never used in the UUID validation logic. The method always asserts that the restored UUID differs from the source UUID, regardless of the parameter value. However, the comment at line 955 explicitly states "restore always creates a fresh index with a NEW UUID", which aligns with the current implementation. The parameter appears to be intended for future extensibility or was left from refactoring. While implementing conditional logic would make the parameter functional, the current behavior is correct for the documented V2 restore semantics.	Medium
	Verify parent-child directory relationship The file counting logic increments the counter for every file that has both indexUUID and categoryDirName in its path components. However, it doesn't verify that categoryDirName appears as a direct parent directory of the file. This could incorrectly count files if the UUID or category name appears elsewhere in the path hierarchy. Verify the parent-child relationship to ensure accurate counting. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [1556-1587] private static long countFilesUnder(Path rootPath, String indexUUID, String categoryDirName) throws IOException { if (Files.exists(rootPath) == false) return 0; long[] count = { 0 }; Files.walkFileTree(rootPath, new java.nio.file.SimpleFileVisitor<>() { @Override public java.nio.file.FileVisitResult visitFile(Path file, java.nio.file.attribute.BasicFileAttributes attrs) { - boolean hasUuid = false; - boolean hasCategory = false; - for (Path part : file) { - String name = part.toString(); - if (indexUUID.equals(name)) { - hasUuid = true; - } else if (categoryDirName.equals(name)) { - hasCategory = true; + Path parent = file.getParent(); + if (parent != null && categoryDirName.equals(parent.getFileName().toString())) { + for (Path part : file) { + if (indexUUID.equals(part.toString())) { + count[0]++; + break; + } } - } - if (hasUuid && hasCategory) { - count[0]++; } return java.nio.file.FileVisitResult.CONTINUE; } Suggestion importance[1-10]: 6 __ Why: The file counting logic iterates through all path components and increments the counter if both indexUUID and categoryDirName appear anywhere in the path. This could incorrectly count files if these strings appear in unrelated path segments. However, the current implementation is likely sufficient for the test's purpose (verifying cleanup of format-specific files under a UUID subtree), and the suggested change to verify direct parent-child relationship may be overly restrictive for path-type-agnostic layouts (FIXED, HASHED_PREFIX, HASHED_INFIX).	Low

Suggestions up to commit a11aab5

Category	Suggestion	Impact
General	Implement unused parameter logic The requireSameUUID parameter is never used in the method body, yet the assertion logic unconditionally expects the restored UUID to differ from the source UUID. This contradicts the parameter's name and the method's contract. Either remove the unused parameter or implement conditional UUID validation based on its value to match the intended behavior. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [955-966] protected void assertRestoredIndexMatches( Client client, String restoredIndexName, IndexShard restoredShard, PreSnapshotState pre, boolean requireSameUUID ) throws IOException { ... - // UUID check: restore always creates a fresh index with a NEW UUID (not the snapshot's source UUID). - // For rename, the new UUID is also different. So in both cases the restored UUID must - // (a) be set, (b) differ from the pre-snapshot source UUID. String restoredUuid = client.admin() .indices() .prepareGetSettings(restoredIndexName) .get() .getSetting(restoredIndexName, IndexMetadata.SETTING_INDEX_UUID); assertNotNull("restored index UUID must be set", restoredUuid); assertFalse("restored index UUID must be non-empty", restoredUuid.isEmpty()); - assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); + if (requireSameUUID) { + assertEquals("restored index must preserve source UUID when requireSameUUID=true", pre.indexUUID, restoredUuid); + } else { + assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); + } ... } Suggestion importance[1-10]: 8 __ Why: The requireSameUUID parameter is declared but never used in the method body. The assertion logic unconditionally expects the restored UUID to differ from the source UUID, which contradicts the parameter's intent. This is a correctness issue that could lead to incorrect test validation when requireSameUUID=true is passed.	Medium
	Verify all format directories cleaned The test validates cleanup of segments/ and translog/ directories but does not verify cleanup of DFA-specific format directories like parquet/. For a test explicitly named to validate "all format files" cleanup, this omission could miss regressions where per-format files are leaked. Add assertions for all expected format directories. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [1541-1548] public void testV2DeleteSnapshotCleansUpAllFormatFilesForDFA() throws Exception { ... assertBusy(() -> { long segCount = countFilesUnder(remoteRepoPath, indexUUID, "segments"); long translogCount = countFilesUnder(remoteRepoPath, indexUUID, "translog"); + long parquetCount = countFilesUnder(remoteRepoPath, indexUUID, "parquet"); assertEquals("segments/ subtree must be cleaned up for indexUUID=" + indexUUID, 0, segCount); assertEquals("translog/ subtree must be cleaned up for indexUUID=" + indexUUID, 0, translogCount); + assertEquals("parquet/ subtree must be cleaned up for indexUUID=" + indexUUID, 0, parquetCount); }, 120, TimeUnit.SECONDS); } Suggestion importance[1-10]: 7 __ Why: The test is explicitly named to validate cleanup of "all format files" for DFA but only checks segments/ and translog/ directories. Missing validation for DFA-specific directories like parquet/ could allow regressions where per-format files leak. Adding this assertion would strengthen the test's coverage of its stated purpose.	Medium
	Optimize file path matching The file-counting logic iterates over all path components for every file, which is inefficient for deep directory trees. Since the method targets a specific indexUUID/categoryDirName structure, convert the file path to a string once and use contains() checks or path-based filtering to reduce overhead in large repositories. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [1556-1587] private static long countFilesUnder(Path rootPath, String indexUUID, String categoryDirName) throws IOException { if (Files.exists(rootPath) == false) return 0; long[] count = { 0 }; + String targetPattern = indexUUID + "/" + categoryDirName; Files.walkFileTree(rootPath, new java.nio.file.SimpleFileVisitor<>() { @Override public java.nio.file.FileVisitResult visitFile(Path file, java.nio.file.attribute.BasicFileAttributes attrs) { - boolean hasUuid = false; - boolean hasCategory = false; - for (Path part : file) { - String name = part.toString(); - if (indexUUID.equals(name)) { - hasUuid = true; - } else if (categoryDirName.equals(name)) { - hasCategory = true; - } - } - if (hasUuid && hasCategory) { + String pathStr = file.toString().replace('\\', '/'); + if (pathStr.contains(targetPattern)) { count[0]++; } return java.nio.file.FileVisitResult.CONTINUE; } ... }); return count[0]; } Suggestion importance[1-10]: 4 __ Why: The suggestion proposes a performance optimization by replacing per-component iteration with string pattern matching. While this could improve efficiency in large repositories, the current implementation is correct and the performance impact is likely minimal in typical test scenarios. The optimization is valid but offers marginal benefit.	Low

Suggestions up to commit d7bd5e2

Category	Suggestion	Impact
Possible issue	Fix UUID assertion logic The UUID assertion logic contradicts the requireSameUUID parameter. When requireSameUUID is true, the method should verify the UUID matches the pre-snapshot UUID, not that it differs. The current implementation always asserts inequality regardless of the parameter value. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [948-959] -protected void assertRestoredIndexMatches( - Client client, - String restoredIndexName, - IndexShard restoredShard, - PreSnapshotState pre, - boolean requireSameUUID -) throws IOException { - ... - // UUID check: restore always creates a fresh index with a NEW UUID (not the snapshot's source UUID). - // For rename, the new UUID is also different. So in both cases the restored UUID must - // (a) be set, (b) differ from the pre-snapshot source UUID. - String restoredUuid = client.admin() - .indices() - .prepareGetSettings(restoredIndexName) - .get() - .getSetting(restoredIndexName, IndexMetadata.SETTING_INDEX_UUID); - assertNotNull("restored index UUID must be set", restoredUuid); - assertFalse("restored index UUID must be non-empty", restoredUuid.isEmpty()); - assertNotEquals("restored index always has a fresh UUID, not the source UUID", pre.indexUUID, restoredUuid); +String restoredUuid = client.admin() + .indices() + .prepareGetSettings(restoredIndexName) + .get() + .getSetting(restoredIndexName, IndexMetadata.SETTING_INDEX_UUID); +assertNotNull("restored index UUID must be set", restoredUuid); +assertFalse("restored index UUID must be non-empty", restoredUuid.isEmpty()); +if (requireSameUUID) { + assertEquals("restored index UUID must match pre-snapshot UUID when requireSameUUID=true", pre.indexUUID, restoredUuid); +} else { + assertNotEquals("restored index always has a fresh UUID when requireSameUUID=false", pre.indexUUID, restoredUuid); +} Suggestion importance[1-10]: 9 __ Why: Critical logic error: the requireSameUUID parameter is completely ignored. The method always asserts UUID inequality regardless of the parameter value, contradicting its documented purpose. This breaks the rename test scenario where requireSameUUID=false is expected.	High
	Correct UUID expectation flag The test passes requireSameUUID=true but the comment in assertRestoredIndexMatches states "restore always creates a fresh index with a NEW UUID". This is contradictory. Based on OpenSearch restore semantics, restored indices receive new UUIDs, so this should be false. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/DataFormatAwareRestoreShallowSnapshotV2IT.java [389] -public void testV2SnapshotCreateAndRestoreForDFAIndex() throws Exception { - ... - assertRestoredIndexMatches(client, indexName, shardAfter, pre, /* requireSameUUID */ true); +assertRestoredIndexMatches(client, indexName, shardAfter, pre, /* requireSameUUID */ false); Suggestion importance[1-10]: 8 __ Why: The test passes requireSameUUID=true but OpenSearch restore semantics always assign new UUIDs to restored indices. This contradicts the inline comment at line 948 stating "restore always creates a fresh index with a NEW UUID". The flag should be false to match actual behavior.	Medium
General	Add deprecation warning log The deprecated 6-arg overload delegates to the 7-arg variant with null IndexMetadata, which prevents DFA format-aware cleanup. Callers using this deprecated method will leak per-format files (e.g., parquet/) on DFA indices. Add a warning log when invoked to alert operators. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java [1345-1354] @Deprecated public static void remoteDirectoryCleanup( RemoteSegmentStoreDirectoryFactory remoteDirectoryFactory, String remoteStoreRepoForIndex, String indexUUID, ShardId shardId, RemoteStorePathStrategy pathStrategy, boolean forceClean ) { + logger.warn("Using deprecated remoteDirectoryCleanup without IndexMetadata; DFA per-format files may not be cleaned for indexUUID={}", indexUUID); remoteDirectoryCleanup(remoteDirectoryFactory, remoteStoreRepoForIndex, indexUUID, shardId, pathStrategy, forceClean, null); } Suggestion importance[1-10]: 5 __ Why: The deprecated 6-arg overload passes null IndexMetadata, preventing DFA format-aware cleanup and leaking per-format files. Adding a warning log helps operators detect this issue, though the core problem (file leakage) remains until callers migrate to the 7-arg variant.	Low
	Detect DFA V1 misconfiguration The comment states DFA indices use V2 only, but there's no runtime guard preventing a DFA index from being associated with a V1 snapshot. If this occurs, per-format files will silently leak. Add an assertion or warning log to detect this misconfiguration. server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java [1705-1716] +if (indexMetadata != null && new IndexSettings(indexMetadata, Settings.EMPTY).isPluggableDataFormatEnabled()) { + logger.warn("V1 shallow-copy cleanup invoked for DFA index {}; per-format files may leak. DFA indices should use V2 snapshots.", indexUUID); +} remoteDirectoryCleanupAsync( remoteDirectoryFactory, threadPool, remoteStoreRepoForIndex, indexUUID, new ShardId(Index.UNKNOWN_INDEX_NAME, indexUUID, Integer.parseInt(shardId)), ThreadPool.Names.REMOTE_PURGE, remoteStoreShardShallowCopySnapshot.getRemoteStorePathStrategy(), false, - null // V1 shallow-copy is a Lucene-only mode — DFA indices use V2 snapshots only. If a DFA index were ever associated with - // a V1 snapshot, per-format files (e.g., parquet/) would leak on cleanup; tracked as a known limitation. + null ); Suggestion importance[1-10]: 4 __ Why: The comment states DFA indices use V2 only, but there's no runtime guard. Adding a warning log helps detect misconfiguration, though the suggestion's check requires indexMetadata which is null in this V1 code path (line 1715), making the proposed guard ineffective without broader refactoring.	Low

[github-actions]

Persistent review updated to latest commit 97d7d61

[github-actions]

❌ Gradle check result for 97d7d61: 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 97d7d61

[github-actions]

❌ Gradle check result for 97d7d61: 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 97d7d61

[github-actions]

Persistent review updated to latest commit ec0519d

[github-actions]

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

[github-actions]

❕ Gradle check result for ec0519d: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

❌ Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.50%. Comparing base (4f9e89c) to head (04e8764).

Files with missing lines	Patch %	Lines
...java/org/opensearch/index/shard/StoreRecovery.java	0.00%	2 Missing ⚠️
...earch/index/store/RemoteSegmentStoreDirectory.java	33.33%	2 Missing ⚠️
...e/metadata/TransportRemoteStoreMetadataAction.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21742      +/-   ##
============================================
+ Coverage     73.46%   73.50%   +0.03%     
- Complexity    75351    75364      +13     
============================================
  Files          6028     6028              
  Lines        341999   342003       +4     
  Branches      49185    49185              
============================================
+ Hits         251257   251379     +122     
+ Misses        70809    70628     -181     
- Partials      19933    19996      +63     

☔ 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.

[github-actions]

Persistent review updated to latest commit 674e230

[github-actions]

❌ Gradle check result for 674e230: 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 f9e0baa

[github-actions]

✅ Gradle check result for f9e0baa: SUCCESS

[github-actions]

Persistent review updated to latest commit 5622ea4

[github-actions]

❕ Gradle check result for 5622ea4: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

Persistent review updated to latest commit a11aab5

[github-actions]

❌ Gradle check result for a11aab5: 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 92d2060

[github-actions]

❌ Gradle check result for 92d2060: 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 92d2060

[github-actions]

✅ Gradle check result for 92d2060: SUCCESS

[github-actions]

Persistent review updated to latest commit 04e8764

[github-actions]

❌ Gradle check result for 04e8764: 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 04e8764

[github-actions]

✅ Gradle check result for 04e8764: SUCCESS