Faster ShardsLimitAllocationDecider#82251
Faster ShardsLimitAllocationDecider#82251original-brownbear merged 5 commits intoelastic:masterfrom original-brownbear:faster-shards-limit-alloc-decider
Conversation
As in many other cases, cache the setting value on `IndexMetadata` and also don't parse the constant setting value from the static settings on every decision and store it in a field instead of the settings.
|
Pinging @elastic/es-distributed (Team:Distributed) |
henningandersen
left a comment
There was a problem hiding this comment.
A few smaller comments, otherwise looking good.
| public ShardsLimitAllocationDecider(Settings settings, ClusterSettings clusterSettings) { | ||
| this.settings = settings; | ||
| this.clusterShardLimit = CLUSTER_TOTAL_SHARDS_PER_NODE_SETTING.get(settings); | ||
| this.indexShardLimit = INDEX_TOTAL_SHARDS_PER_NODE_SETTING.get(settings); |
There was a problem hiding this comment.
I think this is not necessary, this setting is not legal as a node setting.
There was a problem hiding this comment.
Right 🤦 removed this, making the rest of the code far simpler as well.
| private final List<String> tierPreference; | ||
|
|
||
| @Nullable | ||
| private final Integer shardsPerNodeLimit; |
There was a problem hiding this comment.
I think I would prefer to use -1 as no limit since that is the definition on the setting as well. I think an explicit -1 setting value is OK, which would cause this to have a (Integer) -1 value anyway.
Perhaps we need a test with an explicit -1 to verify this case works too (seems we do not have such a test)?
There was a problem hiding this comment.
Test is not necessary thanks to your comment above :) I could just use the setting outright now because we only use the setting and no fallback to node settings so we can use int and have all kinds of tests that test the setting default of -1 :)
|
Jenkins run elasticsearch-ci/part-2 (unrelated Gradle issue) |
|
Thanks Henning, simplified this quite a bit now and should be good for another round :) |
server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java
Outdated
Show resolved
Hide resolved
|
Jenkins run elasticsearch-ci/part-1 (known + unrelated) |
1 similar comment
|
Jenkins run elasticsearch-ci/part-1 (known + unrelated) |
|
Thanks Henning! |
💚 Backport successful
|
As in many other cases, cache the setting value on `IndexMetadata` and also don't parse the constant setting value from the static settings on every decision and store it in a field instead of the settings.
As in many other cases, cache the setting value on `IndexMetadata` and also don't parse the constant setting value from the static settings on every decision and store it in a field instead of the settings.
As in many other cases, cache the setting value on
IndexMetadataand also don't parse the constant setting value from the static
settings on every decision and store it in a field instead of the
settings.
This one is currently the second slowest decider in tests after the disk threshold one and this makes it effectively disappear from profiling.
relates #77466