-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
[ML] indicate overall deployment failure if all node routes are failed #88378
[ML] indicate overall deployment failure if all node routes are failed #88378
Conversation
Pinging @elastic/ml-core (Team:ML) |
Hi @benwtrent, I've created a changelog YAML for you. |
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
@@ -519,7 +519,15 @@ public void writeTo(StreamOutput out) throws IOException { | |||
out.writeOptionalVInt(queueCapacity); | |||
out.writeInstant(startTime); | |||
out.writeList(nodeStats); | |||
out.writeOptionalEnum(state); | |||
if (out.getVersion().onOrAfter(Version.V_8_4_0)) { |
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.
nit: the code could be structure in a way that makes it more explicit that the reason for these checks is that the FAILED
state cannot be streamed to an < 8.4 node.
if (AssignmentState.FAILED.equals(state) && out.getVersion().before(Version.V_8_4_0)) {
out.writeOptionalEnum(AssignmentState.STARTING);
} else {
out.writeOptionalEnum(state);
}
…w-failed-deployment-state
@elasticmachine update branch |
@elasticmachine update branch |
If all node routes are failed, we should indicate that the whole deployment is failed through its assignment state.
The failures could be due to multiple reasons, so for detailed information, the individual node routing reasons should be investigated.