Skip to content

Clamp merge scheduler thread count when it exceeds max merge count - #155693

Open
reugn wants to merge 2 commits into
elastic:mainfrom
reugn:fix/merge-scheduler-max-thread-merge-count
Open

Clamp merge scheduler thread count when it exceeds max merge count#155693
reugn wants to merge 2 commits into
elastic:mainfrom
reugn:fix/merge-scheduler-max-thread-merge-count

Conversation

@reugn

@reugn reugn commented Aug 2, 2026

Copy link
Copy Markdown
Member

Bad index.merge.scheduler combinations could throw while building IndexSettings during cluster-state application (including index deletion), so a node stopped applying later states. The usual case is an explicit max_merge_count with a default max_thread_count derived from the local processor count, which can be higher on some nodes than on the master that accepted the setting. The default also ignored node.processors because it was resolved from index settings alone.

This change clamps max_thread_count to max_merge_count on apply so published settings are always accepted, resolves those defaults from merged node+index settings, and rejects create/update requests only when both values are set explicitly and inverted. include_defaults on get-settings now merges node settings so the reported default matches.

Closes #96594
Closes #155678

@reugn
reugn requested a review from a team as a code owner August 2, 2026 13:28
@reugn reugn added >bug auto-backport Automatically create backport pull requests when merged Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/Search Catch all for Search Foundations v9.4.0 v9.5.0 v9.6.0 labels Aug 2, 2026
@elasticsearchmachine

Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@elasticsearchmachine

Copy link
Copy Markdown
Collaborator

Hi @reugn, I've created a changelog YAML for you.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

✅ Elastic Docs Style Checker (Vale)

No issues found on modified lines!


The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🔍 Preview links for changed docs

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

ℹ️ Important: Docs version tagging

👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version.

We use applies_to tags to mark version-specific features and changes.

Expand for a quick overview

When to use applies_to tags:

✅ At the page level to indicate which products/deployments the content applies to (mandatory)
✅ When features change state (e.g. preview, ga) in a specific version
✅ When availability differs across deployments and environments

What NOT to do:

❌ Don't remove or replace information that applies to an older version
❌ Don't add new information that applies to a specific version without an applies_to tag
❌ Don't forget that applies_to tags can be used at the page, section, and inline level

🤔 Need help?

@reugn
reugn requested review from DaveCTurner and drempapis August 2, 2026 15:34
if (indexToDefaultSettings != null) {
Settings defaultSettings = settingsFilter.filter(indexScopedSettings.diff(indexMetadata.getSettings(), Settings.EMPTY));
// Include node settings so defaults that depend on them (e.g. node.processors) resolve.
final Settings settingsForDefaults = Settings.builder().put(nodeSettings).put(indexMetadata.getSettings()).build();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems deeply suspicious. It doesn't make sense to merge node-scoped settings with index-scoped settings like this.

I'd rather we stuck with today's behaviour in which index.merge.scheduler.max_thread_count is independent of the node-scoped node.processors setting, but clarified this independence thusly:

diff --git a/server/src/main/java/org/elasticsearch/index/MergeSchedulerConfig.java b/server/src/main/java/org/elasticsearch/index/MergeSchedulerConfig.java
index b182bba82733..bd97ad8a2723 100644
--- a/server/src/main/java/org/elasticsearch/index/MergeSchedulerConfig.java
+++ b/server/src/main/java/org/elasticsearch/index/MergeSchedulerConfig.java
@@ -45,7 +45,7 @@ public final class MergeSchedulerConfig {

     public static final Setting<Integer> MAX_THREAD_COUNT_SETTING = new Setting<>(
         "index.merge.scheduler.max_thread_count",
-        (s) -> Integer.toString(Math.max(1, EsExecutors.allocatedProcessors(s) / 2)),
+        (s) -> Integer.toString(Math.max(1, Runtime.getRuntime().availableProcessors() / 2)),
         (s) -> Setting.parseInt(s, 1, "index.merge.scheduler.max_thread_count"),
         Property.Dynamic,
         Property.IndexScope

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, are you good with not honoring node.processors for the thread count default (use availableProcessors() instead)?

If so I'll drop the settings merge in TransportGetSettingsAction, MergeSchedulerConfig, and IndexSettings, and keep this PR to the clamp.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, are you good with not honoring node.processors for the thread count default (use availableProcessors() instead)?

Yes. AFAICT that's how this has behaved for at least a decade. I'm not sure it's ever actually respected node.processors.

);
}
this.maxThreadCount = maxThreadCount;
this.maxThreadCount = Math.min(maxThreadCount, maxMergeCount);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should emit a warning in the logs when this clamping takes effect. It will be surprising to users if they try and increase the max thread count and Elasticsearch just silently ignores them. I'm ok with clamping (i.e. ignoring the invalid config) as long as it's not silent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged >bug :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v9.4.0 v9.5.0 v9.6.0

Projects

None yet

3 participants