Don't overwrite decision with NOT_PREFERRED unless its an improvement#141565
Don't overwrite decision with NOT_PREFERRED unless its an improvement#141565nicktindall merged 10 commits intoelastic:mainfrom
Conversation
nicktindall
added
>bug
:Distributed/Allocation
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Hi @nicktindall, I've created a changelog YAML for you. |
| // Whether or not a relocation target node can be found, it's important to explain the canAllocate response as | ||
| // NOT_PREFERRED, as opposed to NO. | ||
| bestDecision = Type.NOT_PREFERRED; | ||
| if (allocationDecision.type().compareToBetweenNodes(bestDecision) > 0) { |
There was a problem hiding this comment.
I think the whole parent if block become redundant.
if (allocationDecision.type() == Type.NOT_PREFERRED && remainDecision.type() == Type.NOT_PREFERRED) {
The NOT_PREFERRED else-if branch below should handle this case.
There was a problem hiding this comment.
Does this make it right do you think?
We still don't want to set targetNode if remainDecision == NOT_PREFERRED
There was a problem hiding this comment.
It's easier to read and make sense to me: "dont move not-preferred to another not-preferred". Sounds straightforward.
There was a problem hiding this comment.
Updated comment and logic. I think the way you stated it is clearer.
| // We will accept a NOT_PREFERRED allocation if canRemain = NO, but if canRemain = NOT_PREFERRED | ||
| // we will wait for a YES/THROTTLE. Either way we update bestDecision so we can distinguish betweem | ||
| // a NO and a NOT_PREFERRED in allocate-explain | ||
| if (remainDecision.type() != Type.NOT_PREFERRED) { |
There was a problem hiding this comment.
Can we add an assertion like the follows in the beginning of the method
assert remainDecision.type() == Decision.Type.NO || remainDecision.type() == Decision.Type.NOT_PREFERRED : remainDecision;?
It always took me a second to understand why we are not considering more types at this point.
|
I think we should backport to 9.3.1 |
nicktindall
added
auto-backport
💚 Backport successful
|
4 participants
When the not-preferred logging is turned on we re-run
decideMovewith debug mode on. If we encounter aNOT_PREFERREDafter we've seen aYESin debug mode, we were over-writing theYESdecision withNOT_PREFERRED. This change checks first thatNOT_PREFERREDis a better decision than the current one before overwriting it.