Allow index.search.idle.after to be -1 to disable search idle

[tonklable]

Description

[Describe what this change achieves]
Currently, OpenSearch sets index.search.idle.after to 30s by default. To disable idle time, there is only one way, to set index.refresh_interval explicitly. However, index.refresh_interval is 1s by default, and we did not want to change it. It is not a good convention to set one variable explicitly for a different behavior (disabling idle).

-1 is a standard convention to disable something, so this PR is to allow -1 for index.search.idle.after in order to disable idle time.

Related Issues

Resolves #9707 (comment)
I commented in a closed Issue. I can raised a new Issue if needed.

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable. Add doc for allowing -1 index.search.idle.after documentation-website#12992

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 205e782)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 83bc978

[github-actions]

Persistent review updated to latest commit 1681737

[github-actions]

Persistent review updated to latest commit 8eb9008

[github-actions]

❌ Gradle check result for 8eb9008: 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 1a286a0

[github-actions]

❌ Gradle check result for 1a286a0: 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 20fab4c

[github-actions]

❌ Gradle check result for 20fab4c: 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 5ab7fec

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 681cb1a

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Match the documented disable sentinel explicitly Using searchIdleAfter >= 0 still treats a setting of 0 as "always idle", but with TimeValue.MINUS_ONE as the sentinel disabling value, the check should be > 0 only when disabling means negative. Actually, -1 millis returns false correctly here, but be aware that any negative TimeValue would disable idle; consider explicit comparison searchIdleAfter != -1 to match the documented sentinel and avoid disabling on unintended negative durations. server/src/main/java/org/opensearch/index/shard/IndexShard.java [5497-5500] public final boolean isSearchIdle() { long searchIdleAfter = indexSettings.getSearchIdleAfter().getMillis(); - return searchIdleAfter >= 0 && (threadPool.relativeTimeInMillis() - lastSearcherAccess.get()) >= searchIdleAfter; + return searchIdleAfter != -1 && (threadPool.relativeTimeInMillis() - lastSearcherAccess.get()) >= searchIdleAfter; } Suggestion importance[1-10]: 3 __ Why: The suggestion is minor and somewhat debatable. The current >= 0 check works correctly with TimeValue.MINUS_ONE (which is -1 ms), and the minimum bound in the setting definition would prevent other negative values. The change is a minor stylistic preference with low impact.	Low

---

Previous suggestions

Suggestions up to commit 8f5ff50

Category	Suggestion	Impact
General	Ensure all consumers handle negative idle value TimeValue.MINUS_ONE has millis value of -1, but the check searchIdleAfter >= 0 would also treat a legitimate 0ms idle-after as valid (idle immediately). If the intent is to disable only when the setting is -1, use a stricter check such as searchIdleAfter >= 0 is fine, but callers of getSearchIdleAfter() elsewhere (e.g., scheduling delays based on this TimeValue) may still use the negative value and misbehave. Consider guarding all consumers of getSearchIdleAfter() or returning a sentinel handled uniformly. server/src/main/java/org/opensearch/index/shard/IndexShard.java [5497-5500] +public final boolean isSearchIdle() { + long searchIdleAfter = indexSettings.getSearchIdleAfter().getMillis(); + return searchIdleAfter >= 0 && (threadPool.relativeTimeInMillis() - lastSearcherAccess.get()) >= searchIdleAfter; +} - Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about other consumers of getSearchIdleAfter() potentially misbehaving with negative values, but the improved_code is identical to existing_code, providing no actionable change.	Low

Suggestions up to commit 5ab7fec

Category	Suggestion	Impact
General	Preserve zero-value semantics for search idle Using searchIdleAfter >= 0 treats a setting of exactly 0 as "always idle", but the sentinel for disabling should be -1 only. Any negative value from TimeValue.MINUS_ONE.getMillis() (which is -1) should disable, while 0 should preserve the previous "always idle" semantics. Change the condition to > 0 only if 0 should also disable; otherwise use >= 0 but ensure the minimum still allows 0. Given the setting min is now MINUS_ONE, using searchIdleAfter < 0 for the disable check is clearer and preserves the semantics of 0. server/src/main/java/org/opensearch/index/shard/IndexShard.java [5497-5500] public final boolean isSearchIdle() { long searchIdleAfter = indexSettings.getSearchIdleAfter().getMillis(); - return searchIdleAfter >= 0 && (threadPool.relativeTimeInMillis() - lastSearcherAccess.get()) >= searchIdleAfter; + if (searchIdleAfter < 0) { + return false; + } + return (threadPool.relativeTimeInMillis() - lastSearcherAccess.get()) >= searchIdleAfter; } Suggestion importance[1-10]: 4 __ Why: The suggestion is functionally equivalent to the existing code (searchIdleAfter >= 0 and searchIdleAfter < 0 return false yield the same behavior for value 0). It's mainly a readability/clarity refactor rather than a correctness fix.	Low

[github-actions]

Persistent review updated to latest commit 8f5ff50

[github-actions]

Persistent review updated to latest commit 681cb1a

[github-actions]

❌ Gradle check result for 681cb1a: 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 1d28e1f

[github-actions]

❌ Gradle check result for 1d28e1f: 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 1ffbcf1

[github-actions]

❕ Gradle check result for 1ffbcf1: 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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.52%. Comparing base (608d710) to head (205e782).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22873      +/-   ##
============================================
- Coverage     71.58%   71.52%   -0.07%     
+ Complexity    77341    77248      -93     
============================================
  Files          6170     6170              
  Lines        359775   359774       -1     
  Branches      52478    52479       +1     
============================================
- Hits         257541   257313     -228     
- Misses        81770    82005     +235     
+ Partials      20464    20456       -8     

☔ View full report in Codecov by Harness.
📢 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 7e7871d

[github-actions]

❌ Gradle check result for 7e7871d: 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 205e782

[github-actions]

✅ Gradle check result for 205e782: SUCCESS