Undo not-preferred in allocation explain#140195
Closed
DiannaHohensee wants to merge 2 commits intoelastic:mainfrom
Closed
Undo not-preferred in allocation explain#140195DiannaHohensee wants to merge 2 commits intoelastic:mainfrom
DiannaHohensee wants to merge 2 commits intoelastic:mainfrom
Conversation
elasticsearchmachine
added
needs:triage
DiannaHohensee
force-pushed
the
2026/01/05/fix-allocation
branch
from
4947525 to
974cb98
Compare
elasticsearchmachine
added
the
serverless-linked
prdoyle
reviewed
Jan 5, 2026
Contributor
There was a problem hiding this comment.
Verified:
- All the (non-test) code changes are putting the code back the way it was before #137228
Not checked:
- Test code
- That all the changes from #137228 were reverted. I don't understand the changes, so I assume the changes not reverted were omitted deliberately.
Remarks:
- I don't see a new Transport Version yet, nor the corresponding checks.
- We're not changing the
AllocationDecisionIDs back here; I believe that's deliberate.(FWIW, if we're changing them anyway, I'd consider spacing them out (say, counting by fives) so there's space to add new ones inbetween.)
Comment on lines
+534
to
+537
| // rebalance to the node, only will get overwritten if the decision here is to | ||
| // THROTTLE and we get a decision with YES on another node | ||
| bestRebalanceCanAllocateDecisionType = canAllocate.type(); | ||
| if (canAllocate.type().higherThan(Type.NOT_PREFERRED)) { | ||
| // Movement is only allowed to THROTTLE/YES nodes. NOT_PREFERRED is the same as no for rebalancing, since | ||
| // rebalancing aims to distribute resource usage and NOT_PREFERRED means the move could cause hot-spots. | ||
| targetNode = node; | ||
| } | ||
| targetNode = node; |
| // NOT_PREFERRED, as opposed to NO. | ||
| bestDecision = Type.NOT_PREFERRED; | ||
| // Relocating a shard from one NOT_PREFERRED node to another NOT_PREFERRED node would not improve the situation. | ||
| // Relocating a shard from one NOT_PREFERRED node to another would not improve the situation. |
| } else if (bestDecision == Type.THROTTLE) { | ||
| assert allocation.isSimulating() == false; | ||
| // THROTTLE is better than NOT_PREFERRED, we just need to wait for a YES. | ||
| targetNode = null; |
| // but NOT_PREFERRED and THROTTLED are generally not comparable. | ||
| // NOT_PREFERRED and THROTTLED should not be compared. | ||
| THROTTLE, | ||
| NOT_PREFERRED, |
Contributor
There was a problem hiding this comment.
Do these get serialized over Transport? If so, are they sent by name or by ordinal?
I ask because if it's by ordinal, then reordering these would be a breaking change.
(From here)
Contributor
Author
There was a problem hiding this comment.
I'm not sure about those specifics, but I do think the original change had a backwards compatibility bug: I should have changed the writeTo / readFrom methods, needing a transport version gate.
| */ | ||
| public boolean assignmentAllowed() { | ||
| return this == NOT_PREFERRED || this == YES; | ||
| return this.compareTo(NOT_PREFERRED) >= 0; |
| case YES -> YES; | ||
| case NOT_PREFERRED -> NOT_PREFERRED; | ||
| // TODO: this | ||
| case YES, NOT_PREFERRED -> YES; |
Contributor
There was a problem hiding this comment.
From here.
The TODO comment isn't right, but presumably this is temporary.
| // the final decision is NO (no node to move the shard to) and we are not in explain mode, return a cached version | ||
| return CACHED_CANNOT_MOVE_DECISION; | ||
| } else { | ||
| assert ((targetNode == null) == (moveDecision != AllocationDecision.YES)); |
| : Explanations.Rebalance.CANNOT_REBALANCE_CANNOT_ALLOCATE; | ||
| case THROTTLE -> Explanations.Rebalance.CLUSTER_THROTTLE; | ||
| case YES -> { | ||
| case YES, NOT_PREFERRED -> { |
| case NO -> canRemainNotPreferred() ? Explanations.Move.NOT_PREFERRED_TO_NO : Explanations.Move.NO; | ||
| case YES, NOT_PREFERRED -> Explanations.Move.YES; | ||
| case THROTTLED -> Explanations.Move.THROTTLED; | ||
| case NO -> Explanations.Move.NO; |
DiannaHohensee
added
>non-issue
:Distributed/Allocation
Collaborator
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
Contributor
Author
|
We decided to fix the problem instead of rolling it back / undoing it. Closing this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The work adding not-preferred to the allocation explain output
(#137228) resulted in shards not evacuating a shutting down node
during release testing. There is some unidentified bug in play. Since
a transport version was added with that code change, we wish to
retain the enum changes necessitating the transport version, but
undo / revert all logic changes associated with the commit.
Relates ES-13903