-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Remove nesting from multi allocation decision #130844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| } | ||
|
|
||
| final Decision decision = new Decision.Multi().add(allocationDecision).add(rebalanceDecision); | ||
| final Decision.Type canAllocateOrRebalance = Decision.Type.min(allocationDecision.type(), rebalanceDecision.type()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this place could create nested multi decision
DiannaHohensee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| return true; | ||
| } | ||
| return false; | ||
| } else return other == THROTTLE && this == YES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slight nit that this isn't our usual style for if-else statements, skipping the {} brackets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm intellij auto-format did that and spotless didn't complain either.
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you remove this because equality is unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
records implement hash/equals/toString https://docs.oracle.com/en/java/javase/17/language/records.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep toString for bwc. If exact format is not important can remove it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a change might affect the allocation explain response? I'd leave it for bwc, as you say.
…king * upstream/main: (33 commits) Allow both WithEntitlementsOnTestCode and EntitledTestPackages together (elastic#130826) Move streams status actions to cluster:monitor group (elastic#131015) Update JDK base image for OIDC fixture (elastic#131176) Mute org.elasticsearch.xpack.esql.ccq.MultiClustersIT testLookupJoinAliases elastic#131166 Mute org.elasticsearch.index.engine.ThreadPoolMergeExecutorServiceDiskSpaceTests testEnqueuedMergeTasksAreUnblockedWhenEstimatedMergeSizeChanges elastic#131165 Mute org.elasticsearch.xpack.esql.ccq.MultiClustersIT testNotLikeListKeyword elastic#131155 Mute org.elasticsearch.xpack.esql.qa.multi_node.GenerativeIT test elastic#131154 Check file entitlements on the Lucene FilterFileSystem in tests (elastic#130825) Mute org.elasticsearch.xpack.esql.qa.multi_node.EsqlSpecIT test {lookup-join.MvJoinKeyOnFromAfterStats ASYNC} elastic#131148 Move FrequencyCappedAction to common package (elastic#131060) Mute org.elasticsearch.xpack.esql.action.CrossClusterAsyncQueryStopIT testStopQueryLocal elastic#121672 Remove nesting from multi allocation decision (elastic#130844) Disable async search rest tests in release builds (elastic#131132) Fix testStopQueryLocal (elastic#131130) Fixes based on resharding disruption tests (elastic#130870) Remove inactive logger (elastic#131121) Add wait for remote start for the test (elastic#131124) Add existing shards allocator settings to failure store allowed list. (elastic#131056) Don't allow field caps to use semantic queries as index filters (elastic#131111) issue should be already fixed by elastic#121466 (elastic#130860) ...
Refactor
org.elasticsearch.cluster.routing.allocation.decider.Decision. Change to sealed interface and use records for Single and Multi. Also remove recursive Multi decision support.