Make stale segments cleanup logic depend on map size as well

[rayshrey]

Description

Currently stale segment cleanup logic is triggered only during the first refresh after a flush.
With this PR we are adding an additional setting that the cleanup can also get triggered when the map reaches a specific threshold. This threshold is backed by a setting, which can be set to -1 as well to disable this entire flow and fallback to the previous logic.

Related Issues

Resolves #20960

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 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add uploadedSegmentsCleanupThreshold setting Relevant files: server/src/main/java/org/opensearch/indices/RemoteStoreSettings.java server/src/main/java/org/opensearch/common/settings/ClusterSettings.java server/src/test/java/org/opensearch/indices/RemoteStoreSettingsDynamicUpdateTests.java Sub-PR theme: Use threshold setting to trigger stale segment cleanup Relevant files: server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java

⚡ Recommended focus areas for review Weak Test Assertion testCleanupTriggeredWhenMapExceedsThreshold asserts only that the map size is less than 100 after 100 iterations with a threshold of 10. This is a very loose bound that doesn't verify the cleanup actually fired at the right time. The test would pass even if cleanup never triggered but the map happened to stay small for other reasons. A stronger assertion would verify the map size stays near or below the threshold (e.g., mapSize <= threshold + some_small_buffer). public void testCleanupTriggeredWhenMapExceedsThreshold() throws IOException { RemoteSegmentStoreDirectory remoteSegmentStoreDirectory = setupDirectoryWithThreshold(10); indexAndRefreshWithoutFlush(100); int mapSize = remoteSegmentStoreDirectory.getSegmentsUploadedToRemoteStoreSize(); assertTrue("Map size should be reasonable with threshold cleanup, but was: " + mapSize, mapSize < 100); } Unreliable Test testCleanupNotTriggeredWhenThresholdDisabled asserts that finalMapSize > initialMapSize, but initialMapSize is captured before indexAndRefreshWithoutFlush is called. If the initial map is already populated (e.g., from indexDocs(1, 3) + refresh in setupDirectoryWithThreshold), the test may still pass vacuously. More critically, the test uses remoteStoreRefreshListener which was constructed with spyShard pointing to mockSettings returning threshold=-1, but the remoteSegmentStoreDirectory is obtained from indexShard (not spyShard). If the listener uses a different directory instance than the one being measured, the size check is meaningless. public void testCleanupNotTriggeredWhenThresholdDisabled() throws IOException { RemoteSegmentStoreDirectory remoteSegmentStoreDirectory = setupDirectoryWithThreshold(-1); int initialMapSize = remoteSegmentStoreDirectory.getSegmentsUploadedToRemoteStoreSize(); indexAndRefreshWithoutFlush(100); int finalMapSize = remoteSegmentStoreDirectory.getSegmentsUploadedToRemoteStoreSize(); assertTrue( "Map size should have grown with threshold disabled, initial=" + initialMapSize + " final=" + finalMapSize, finalMapSize > initialMapSize ); } Missing Validation The setting CLUSTER_REMOTE_UPLOADED_SEGMENTS_CLEANUP_THRESHOLD_SETTING uses Setting.intSetting with minimum value -1, which correctly rejects values below -1. However, there is no upper bound validation. A very small positive value (e.g., 1 or 2) would cause cleanup to trigger on nearly every refresh cycle, potentially causing excessive remote store operations and performance degradation. Consider adding a reasonable minimum positive value or documenting this risk. public static final Setting<Integer> CLUSTER_REMOTE_UPLOADED_SEGMENTS_CLEANUP_THRESHOLD_SETTING = Setting.intSetting( "cluster.remote_store.uploaded_segments_cleanup_threshold", 10000, -1, Property.NodeScope, Property.Dynamic );

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate setting rejects invalid negative values The Setting.intSetting with a minimum of -1 will allow values like -2, -3, etc., which are semantically invalid (only -1 is a special "disabled" value). A custom validator should be added to reject values between -2 and 0 (exclusive), or the setting should use a validator that only permits values >= 1 or exactly -1. server/src/main/java/org/opensearch/indices/RemoteStoreSettings.java [182-188] public static final Setting<Integer> CLUSTER_REMOTE_UPLOADED_SEGMENTS_CLEANUP_THRESHOLD_SETTING = Setting.intSetting( "cluster.remote_store.uploaded_segments_cleanup_threshold", 10000, -1, + value -> { + if (value < -1 || value == 0) { + throw new IllegalArgumentException( + "cluster.remote_store.uploaded_segments_cleanup_threshold must be -1 (disabled) or a positive integer, got: " + value + ); + } + }, Property.NodeScope, Property.Dynamic ); Suggestion importance[1-10]: 7 __ Why: The current Setting.intSetting with minimum -1 allows values like -2, -3, etc. which are semantically invalid. The test in RemoteStoreSettingsDynamicUpdateTests actually tests that -5 throws an IllegalArgumentException, but the current implementation with min=-1 would allow -2 through -1. A custom validator is needed to only permit -1 or positive integers, making this a real correctness issue.	Medium
	Ensure test asserts on correct directory instance The RemoteSegmentStoreDirectory is retrieved from indexShard.remoteStore().directory() but the listener operates on spyShard. If the spy wraps the shard and the remote directory is obtained differently inside the listener, the test may be asserting on a different map instance than the one being modified. Ensure the directory returned is the exact same instance used by the listener, or retrieve it from spyShard instead. server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java [962-970] -remoteStoreRefreshListener = new RemoteStoreRefreshListener( - spyShard, - SegmentReplicationCheckpointPublisher.EMPTY, - tracker, - mockSettings -); - -return (RemoteSegmentStoreDirectory) ((FilterDirectory) ((FilterDirectory) indexShard.remoteStore().directory()).getDelegate()) +return (RemoteSegmentStoreDirectory) ((FilterDirectory) ((FilterDirectory) spyShard.remoteStore().directory()).getDelegate()) .getDelegate(); Suggestion importance[1-10]: 6 __ Why: The test retrieves RemoteSegmentStoreDirectory from indexShard but the listener uses spyShard. Since spyShard is a Mockito spy wrapping indexShard with getRemoteStoreSettings() overridden, the underlying remoteStore() should be the same instance. However, using spyShard.remoteStore().directory() would be more consistent and safer to ensure the same directory instance is being asserted on.	Low
General	Fix unreliable test assertion for disabled threshold The test captures initialMapSize before calling indexAndRefreshWithoutFlush, but setupDirectoryWithThreshold already calls indexDocs and refresh internally, so initialMapSize may already be non-zero. More importantly, the assertion only checks that the map grew, but doesn't verify that cleanup was NOT triggered (i.e., the map could still be cleaned up partially). The test should assert that the map size is at least as large as expected without any cleanup, or verify that deleteStaleSegmentsAsync was never called. server/src/test/java/org/opensearch/index/shard/RemoteStoreRefreshListenerTests.java [917-928] public void testCleanupNotTriggeredWhenThresholdDisabled() throws IOException { RemoteSegmentStoreDirectory remoteSegmentStoreDirectory = setupDirectoryWithThreshold(-1); - int initialMapSize = remoteSegmentStoreDirectory.getSegmentsUploadedToRemoteStoreSize(); indexAndRefreshWithoutFlush(100); int finalMapSize = remoteSegmentStoreDirectory.getSegmentsUploadedToRemoteStoreSize(); + // With threshold disabled, map should have grown significantly (no threshold-based cleanup) assertTrue( - "Map size should have grown with threshold disabled, initial=" + initialMapSize + " final=" + finalMapSize, - finalMapSize > initialMapSize + "Map size should be large with threshold disabled, but was: " + finalMapSize, + finalMapSize >= 10 ); } Suggestion importance[1-10]: 4 __ Why: The test's assertion that finalMapSize > initialMapSize is somewhat weak since initialMapSize may already be non-zero after setupDirectoryWithThreshold. The improved code simplifies the assertion, but the suggestion's claim about partial cleanup not being verified is valid. However, the improved code still doesn't definitively verify that deleteStaleSegmentsAsync was never called.	Low

[github-actions]

❌ Gradle check result for 1505def: 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 e0c4218: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.31%. Comparing base (85113a4) to head (29bcca9).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #20976   +/-   ##
=========================================
  Coverage     73.31%   73.31%           
- Complexity    72544    72615   +71     
=========================================
  Files          5819     5819           
  Lines        331399   331411   +12     
  Branches      47887    47888    +1     
=========================================
+ Hits         242955   242976   +21     
- Misses        68935    68945   +10     
+ Partials      19509    19490   -19     

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

Failed to generate code suggestions for PR

[github-actions]

✅ Gradle check result for 29bcca9: SUCCESS