Separate shard limit validation for index and search tiers #136063
Conversation
…ex-search-for-shard-limit-validator
| public enum ResultGroup { | ||
| NORMAL(NORMAL_GROUP), | ||
| FROZEN(FROZEN_GROUP), | ||
| INDEX("index"), | ||
| SEARCH("search"); |
There was a problem hiding this comment.
The PR is bigger and more involved than I initially expected because the current shard limit validation has a hard-coded 2-member group for "normal" and "frozen" indices. The group is also decided by an index level setting. None of these makes sense in Stateless.
The PR introduces ResultGroup so that the actual groups can be picked based on the setup. It also helps detaching the grouping from the index level setting. Overall it promotes the Group concept (was a String previously) which in turns helps reusing the existing logics (many of them are based on the group in use).
Please let me know if it makes sense. Happy to provide more clarification.
There was a problem hiding this comment.
Is it worth using SPI to provide the ResultGroups so serverless can override it and we avoid putting knowledge of those things in the core product?
It may be pedantic and not worth the effort, but it looks generalised enough that it could be done. And we do do it for some other things.
I guess ResultGroup would have to be an interface then
There was a problem hiding this comment.
This is a good question and I did briefly consider it initially. Didn't go with it because (1) relatively larger effort (2) there is a bit untangle needed for ShardsCapacityHealthIndicatorService (3) a bit easier to test everything together with existing testing code.
Another reason is that focus of this PR is a bug fix on the stateless side and SPI feels more of an optimization. So I think pursuing SPI separately might be a better option. I can raise a separate ticket for it. Let me know if this makes sense or you'd prefer we pursue it as part of this PR.
There was a problem hiding this comment.
Yeah I wouldn't block this on account of the SPI, I'll leave it up to you whether you think it's worth a follow up.
There was a problem hiding this comment.
I'll leave this PR open till tomorrow. If no objection by then, I'll log a ticket for future optimization with SPI and merge this PR so that we can get the bug fix part shipped.
ywangd
added
>non-issue
:Distributed Coordination/Allocation
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| final var resultGroups = applicableResultGroups(isStateless); | ||
| final Map<ResultGroup, Integer> shardsToCreatePerGroup = new HashMap<>(); | ||
|
|
||
| // TODO: we can short circuit when indindicesToOpenices is empty |
There was a problem hiding this comment.
Nit: typo indindicesToOpenices, also did you mean to act on this TODO before merging?
There was a problem hiding this comment.
The TODO is for future since I intend to keep the current behaviour as is. Fixed the typo in 241bf1b
| * - otherwise -> returns the Result of checking the limits for _frozen_ nodes | ||
| * - Check limits for _normal_ nodes | ||
| * - If there's no room -> return the Result for _normal_ nodes (fail-fast) | ||
| * - otherwise -> returns the Result of checking the limits for _frozen_ nodes |
There was a problem hiding this comment.
Nit: this javadoc probably needs to be generalised
There was a problem hiding this comment.
Yep generalized in 241bf1b
| public enum ResultGroup { | ||
| NORMAL(NORMAL_GROUP), | ||
| FROZEN(FROZEN_GROUP), | ||
| INDEX("index"), | ||
| SEARCH("search"); |
There was a problem hiding this comment.
Is it worth using SPI to provide the ResultGroups so serverless can override it and we avoid putting knowledge of those things in the core product?
It may be pedantic and not worth the effort, but it looks generalised enough that it could be done. And we do do it for some other things.
I guess ResultGroup would have to be an interface then
| case FROZEN -> nodeCount(discoveryNodes, ShardLimitValidator::hasFrozen); | ||
| case INDEX -> nodeCount(discoveryNodes, node -> node.hasRole(DiscoveryNodeRole.INDEX_ROLE.roleName())); | ||
| case SEARCH -> nodeCount(discoveryNodes, node -> node.hasRole(DiscoveryNodeRole.SEARCH_ROLE.roleName())); | ||
| }; |
There was a problem hiding this comment.
Nit: could make these abstract in the enum class and put the implementations on the individual declarations? that would avoid the need for the switch. Same as below.
There was a problem hiding this comment.
Good point. Done in 035f2ef
| * @return The total number of new shards to be created for this group. | ||
| */ | ||
| public int newShardsTotal(Settings indexSettings) { | ||
| final boolean frozen = FROZEN_GROUP.equals(INDEX_SETTING_SHARD_LIMIT_GROUP.get(indexSettings)); |
There was a problem hiding this comment.
Nit: this is a bit tricky to read, perhaps inFrozenLimitGroup instead of frozen or something?
There was a problem hiding this comment.
I renamed it to isFrozenIndex. I didn't include group since the enum (this) in the context is the group.
| + ReferenceDocs.MAX_SHARDS_PER_NODE; | ||
| } | ||
|
|
||
| public enum ResultGroup { |
There was a problem hiding this comment.
Nit: perhaps LimitGroup? I'm not entirely clear why "result" is in the name? perhaps I'm missing something
There was a problem hiding this comment.
Changed it to LimitGroup which seems to be overall better, see 0dcd553. The Result part was taken from the validation Result inner class name which has a group field of the enum type (previously a String).
…ex-search-for-shard-limit-validator
|
@elasticmachine update branch |
In stateless, index and search shards are distinct and must be allocated to nodes of corresponding types. Therefore the shard limit validation should be performed for them separately to avoid one shard type taking more quota than expected, similar to the separation between regular and frozen shards.
Resolves: ES-12884