Add cluster.blocks.read.auto_release setting (#20539)#20582
Add cluster.blocks.read.auto_release setting (#20539)#20582Seungmin123 wants to merge 1 commit intoopensearch-project:mainfrom
Conversation
The DiskThresholdMonitor was unconditionally auto-releasing index.blocks.read on all indices. This fix adds a new setting cluster.blocks.read.auto_release (defaulting to true) to control this behavior, allowing users to disable automatic release of manually-set read blocks. Signed-off-by: Seungmin123 <dltmdals147@gmail.com>
github-actions
bot
added
bug
📝 WalkthroughWalkthroughAdded a new cluster setting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@server/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitor.java`:
- Around line 497-509: The current logic skips releasing any read blocks when
diskThresholdSettings.isIndexReadBlockAutoReleaseEnabled() is false, which also
prevents releasing system-set read blocks; update the computation of
indicesToReleaseReadBlock to exclude indices in indicesNotToAutoRelease rather
than returning Set.of() when auto-release is disabled and also add that
exclusion filter in the enabled branch: inside the Stream for
indicesToReleaseReadBlock (and in the else case), filter out indices where
indicesNotToAutoRelease.contains(index) == true so only indices not protected by
indicesNotToAutoRelease are eligible for auto-release; refer to
diskThresholdSettings.isIndexReadBlockAutoReleaseEnabled(),
indicesToReleaseReadBlock, indicesNotToAutoRelease and mirror the conditional
behavior used by handleReadOnlyBlocks/onNewInfo.
🧹 Nitpick comments (1)
server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitorTests.java (1)
545-659: Tests are correct and cover both scenarios well.Two optional suggestions:
Extract shared setup — both tests duplicate the cluster state construction (~20 lines). A private helper like
buildClusterStateWithReadBlock(String indexName)would reduce noise.Use the existing
clusterInfo()helper (line 1497) instead of directly constructingClusterInfo(lines 595, 655). The helper is used consistently in all other tests in this file.♻️ Suggested simplification for ClusterInfo construction
- monitor.onNewInfo(new org.opensearch.cluster.ClusterInfo(usages, Map.of(), Map.of(), Map.of(), Map.of(), Map.of(), Map.of())); + monitor.onNewInfo(clusterInfo(usages));Apply in both tests (lines 595 and 655).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdserver/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitor.javaserver/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettings.javaserver/src/main/java/org/opensearch/common/settings/ClusterSettings.javaserver/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitorTests.javaserver/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T17:40:27.167Z
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.
Applied to files:
server/src/main/java/org/opensearch/common/settings/ClusterSettings.javaserver/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettings.javaserver/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitor.java
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitorTests.java (1)
server/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettings.java (1)
DiskThresholdSettings(56-537)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code-Diff-Analyzer / Code-Diff-Analyzer
🔇 Additional comments (6)
CHANGELOG.md (1)
16-16: LGTM!Changelog entry is correctly placed under "Fixed" and references the right issue.
server/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettings.java (1)
117-122: LGTM!The new setting definition correctly mirrors the existing
CLUSTER_CREATE_INDEX_BLOCK_AUTO_RELEASEpattern —boolSetting, defaulttrue,Dynamic,NodeScope. Constructor initialization, update consumer registration, and getter/setter are all consistent with the established conventions in this class.server/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitor.java (1)
496-509: LGTM — conditional auto-release logic is correct.The guard on
isIndexReadBlockAutoReleaseEnabled()cleanly mirrors the approach used forisCreateIndexBlockAutoReleaseEnabled()inhandleClusterCreateIndexBlocks. When disabled,Set.of()prevents any auto-release; when enabled, the existing stream filter logic is preserved.server/src/main/java/org/opensearch/common/settings/ClusterSettings.java (1)
363-363: LGTM!Correctly registered alongside the existing
CLUSTER_CREATE_INDEX_BLOCK_AUTO_RELEASEsetting.server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdSettingsTests.java (2)
61-61: LGTM!Default assertion is correctly added to verify
isIndexReadBlockAutoReleaseEnabled()returnstrue.
66-78: LGTM!Clean test for dynamic update of the new setting. Follows the established test pattern in this file.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| final Set<String> indicesToReleaseReadBlock; | ||
| if (diskThresholdSettings.isIndexReadBlockAutoReleaseEnabled()) { | ||
| indicesToReleaseReadBlock = StreamSupport.stream( | ||
| Spliterators.spliterator(state.routingTable().indicesRouting().entrySet(), 0), | ||
| false | ||
| ) | ||
| .map(Map.Entry::getKey) | ||
| .filter(index -> indicesToBlockRead.contains(index) == false) | ||
| .filter(index -> state.getBlocks().hasIndexBlock(index, IndexMetadata.INDEX_READ_BLOCK)) | ||
| .collect(Collectors.toSet()); | ||
| } else { | ||
| indicesToReleaseReadBlock = Set.of(); | ||
| } |
There was a problem hiding this comment.
Setting auto_release=false also suppresses release of system-set read blocks (warm node file cache).
When disabled, this prevents auto-release of all index.blocks.read blocks — including those set by the system when warm nodes breach the file cache search threshold (lines 190-195 in onNewInfo). If a warm node temporarily exceeds the threshold and later recovers, the read block will remain stuck until manually removed or the setting is re-enabled.
Consider whether this is the intended behavior. The analogous handleReadOnlyBlocks method uses the indicesNotToAutoRelease set to selectively protect system-set blocks while still allowing release of others. A similar approach here — only auto-releasing blocks on indices not in indicesNotToAutoRelease — would allow disabling auto-release of manually-set blocks while still permitting system-set blocks to be released.
🤖 Prompt for AI Agents
In
`@server/src/main/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitor.java`
around lines 497 - 509, The current logic skips releasing any read blocks when
diskThresholdSettings.isIndexReadBlockAutoReleaseEnabled() is false, which also
prevents releasing system-set read blocks; update the computation of
indicesToReleaseReadBlock to exclude indices in indicesNotToAutoRelease rather
than returning Set.of() when auto-release is disabled and also add that
exclusion filter in the enabled branch: inside the Stream for
indicesToReleaseReadBlock (and in the else case), filter out indices where
indicesNotToAutoRelease.contains(index) == true so only indices not protected by
indicesNotToAutoRelease are eligible for auto-release; refer to
diskThresholdSettings.isIndexReadBlockAutoReleaseEnabled(),
indicesToReleaseReadBlock, indicesNotToAutoRelease and mirror the conditional
behavior used by handleReadOnlyBlocks/onNewInfo.
|
This PR is stalled because it has been open for 30 days with no activity. |
1 participant
Description
This PR fixes a bug in the
DiskThresholdMonitorwhere it was unconditionally auto-releasingindex.blocks.readon all indices, even when manually set by users.This fix introduces a new dynamic cluster setting
cluster.blocks.read.auto_release(defaulting totruefor backward compatibility) that controls whether the cluster should automatically release read blocks. By setting this tofalse, users can ensure that manually-set read blocks persist until explicitly removed.Related Issues
Resolves #20539
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.