Fix O(n^2) removeAll in remote translog metadata cleanup

[gbbafna]

Description

ArrayList.removeAll(ArrayList) in RemoteFsTimestampAwareTranslog is O(n*m) because ArrayList.contains() is a linear scan. When the number of translog metadata files grows large (e.g., 500+ files for long-lived system indices like .opendistro-ism-config), this causes sustained CPU spikes (500ms at 99.9% CPU) on the remote_purge thread.

Stack trace observed:

ArrayList.indexOfRange → ArrayList.indexOf → ArrayList.contains → ArrayList.batchRemove → ArrayList.removeAll
  → RemoteFsTimestampAwareTranslog$1.onResponse

Hot threads showing 100% cpu

100.4% (502ms out of 500ms) cpu usage by thread 'opensearch[abc][remote_purge][T#1]'
     10/10 snapshots sharing following 20 elements
       java.base@21.0.9/java.util.ArrayList.indexOfRange(ArrayList.java:299)
       java.base@21.0.9/java.util.ArrayList.indexOf(ArrayList.java:286)
       java.base@21.0.9/java.util.ArrayList.contains(ArrayList.java:275)
       java.base@21.0.9/java.util.ArrayList.batchRemove(ArrayList.java:911)
       java.base@21.0.9/java.util.ArrayList.removeAll(ArrayList.java:873)
       app//org.opensearch.index.translog.RemoteFsTimestampAwareTranslog$1.onResponse(RemoteFsTimestampAwareTranslog.java:208)
       app//org.opensearch.index.translog.RemoteFsTimestampAwareTranslog$1.onResponse(RemoteFsTimestampAwareTranslog.java:173)
       app//org.opensearch.common.blobstore.EncryptedBlobContainer.lambda$listBlobsByPrefixInSortedOrder$8(EncryptedBlobContainer.java:266)
       app//org.opensearch.common.blobstore.EncryptedBlobContainer$$Lambda/0x0000000203720698.accept(Unknown Source)

Solution

Wrap the List argument in HashSet before passing to removeAll(), reducing lookup complexity from O(n) to O(1). This fixes 3 occurrences in the file. A 4th removeAll call already uses a Set argument and is unaffected.

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 f395fcc)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Fix O(n^2) removeAll by wrapping lists in HashSet Relevant files: server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java Sub-PR theme: Enhance debug logging with file counts for metadata cleanup Relevant files: server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java

⚡ Recommended focus areas for review Debug Log Cost The new debug log statements include the full list contents (e.g., metadataFilesToBeDeleted and metadataFilesNotToBeDeleted). When these lists are large (500+ entries, which is exactly the problematic scenario this PR targets), constructing these log strings via string concatenation inside the lambda could itself be expensive, even if the debug level is disabled — the lambda defers evaluation, but if debug logging IS enabled, it will serialize large lists. Consider whether logging full list contents at debug level is appropriate for production use cases with large file counts. logger.debug( () -> "metadataFilesToBeDeleted count = " + metadataFilesToBeDeleted.size() + ", metadataFilesToBeDeleted = " + metadataFilesToBeDeleted ); // For all the files that we are keeping, fetch min and max generations List<String> metadataFilesNotToBeDeleted = new ArrayList<>(metadataFiles); metadataFilesNotToBeDeleted.removeAll(new HashSet<>(metadataFilesToBeDeleted)); logger.debug( () -> "metadataFilesNotToBeDeleted count = " + metadataFilesNotToBeDeleted.size() + ", metadataFilesNotToBeDeleted = " + metadataFilesNotToBeDeleted ); Unnecessary HashSet Copy In getMetadataFilesToBeDeleted, metadataFilesContainingMinGenerationToKeep is built from a stream filter and then wrapped in a new HashSet<>() for removeAll. Since the stream result is only used for the removeAll call and the subsequent trace log, it could be collected directly into a HashSet (via Collectors.toSet()) to avoid creating two collections. This is a minor efficiency concern but consistent with the PR's optimization goal. List<String> metadataFilesContainingMinGenerationToKeep = metadataFilesToBeDeleted.stream().filter(md -> { long maxGeneration = TranslogTransferMetadata.getMaxGenerationFromFileName(md); return maxGeneration == -1 || maxGeneration >= minGenerationToKeepInRemote; }).collect(Collectors.toList()); metadataFilesToBeDeleted.removeAll(new HashSet<>(metadataFilesContainingMinGenerationToKeep));

[github-actions]

Persistent review updated to latest commit b657f29

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f395fcc
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use stream filtering instead of removeAll for efficiency Converting metadataFilesToBeDeleted to a HashSet improves lookup performance, but metadataFilesNotToBeDeleted is still an ArrayList, so removeAll iterates over each element of the list and checks membership in the set. For large lists, consider initializing metadataFilesNotToBeDeleted as a HashSet or using stream filtering to build the result directly, which would be more efficient overall. server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java [213] -metadataFilesNotToBeDeleted.removeAll(new HashSet<>(metadataFilesToBeDeleted)); +List<String> metadataFilesNotToBeDeleted = metadataFiles.stream() + .filter(f -> !metadataFilesToBeDeletedSet.contains(f)) + .collect(Collectors.toList()); Suggestion importance[1-10]: 3 __ Why: The suggestion proposes using stream filtering instead of removeAll with a HashSet, but the current approach of wrapping in a HashSet already provides O(1) lookups for the removeAll operation, making the performance difference marginal. The improved_code also introduces an undefined variable metadataFilesToBeDeletedSet that doesn't exist in the PR code.	Low

---

Previous suggestions

Suggestions up to commit b657f29

Category	Suggestion	Impact
General	Optimize list filtering to true O(n) Converting metadataFilesToBeDeleted to a HashSet for the removeAll call is correct for O(n) lookup, but metadataFilesNotToBeDeleted is still an ArrayList, so the overall operation remains O(n*m) due to repeated contains checks inside removeAll on the list side. Consider initializing metadataFilesNotToBeDeleted as a LinkedHashSet (to preserve order) and then converting back to a list if needed, or use metadataFiles.stream().filter(f -> !deleteSet.contains(f)).collect(Collectors.toList()) for a truly O(n) solution. server/src/main/java/org/opensearch/index/translog/RemoteFsTimestampAwareTranslog.java [213] -metadataFilesNotToBeDeleted.removeAll(new HashSet<>(metadataFilesToBeDeleted)); +Set<String> deleteSet = new HashSet<>(metadataFilesToBeDeleted); +List<String> metadataFilesNotToBeDeleted = metadataFiles.stream() + .filter(f -> !deleteSet.contains(f)) + .collect(Collectors.toList()); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that wrapping metadataFilesToBeDeleted in a HashSet for removeAll doesn't fully optimize the operation since ArrayList.removeAll still iterates and checks containment. However, this is a minor performance optimization for what is likely a small list of metadata files, and the improvement would be marginal in practice.	Low

[github-actions]

✅ Gradle check result for b657f29: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 26.66667% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.46%. Comparing base (436e4a6) to head (f395fcc).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...index/translog/RemoteFsTimestampAwareTranslog.java	26.66%	11 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21350      +/-   ##
============================================
+ Coverage     73.34%   73.46%   +0.11%     
- Complexity    74223    74273      +50     
============================================
  Files          5958     5958              
  Lines        337309   337357      +48     
  Branches      48664    48687      +23     
============================================
+ Hits         247408   247841     +433     
+ Misses        70188    69744     -444     
- Partials      19713    19772      +59     

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

[Bukhtawar]

How do we ensure future consumers of metadataFiles handle this correctly. Can we open a TODO on refactoring the class structure broadly

[github-actions]

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

❕ Gradle check result for f395fcc: 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.