- Notifications
You must be signed in to change notification settings - Fork 25.6k
SIGTERM node shutdown type #95430
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
SIGTERM node shutdown type #95430
Conversation
| Pinging @elastic/es-core-infra (Team:Core/Infra) |
| Hi @stu-elastic, 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.
Mostly looks good, left a couple nitpicks and one concern about serialization.
| RESTART, | ||
| REPLACE; | ||
| REPLACE, | ||
| SIGTERM; // local version of REMOVE |
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.
| SIGTERM; // local version of REMOVE | |
| SIGTERM; // locally-initiated version of REMOVE |
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.
Updated.
| singleNodeShutdown -> singleNodeShutdown.getType() == SingleNodeShutdownMetadata.Type.REMOVE | ||
| || singleNodeShutdown.getType() == SingleNodeShutdownMetadata.Type.SIGTERM | ||
| || singleNodeShutdown.getType() == SingleNodeShutdownMetadata.Type.REPLACE |
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.
Nitpick not entirely related to this PR: Can we replace this with a switch expression so that this triggers totality checks in the future?
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.
Replaced.
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.
Can you add some transport version guards in the writeTo method of this class? We shouldn't ever be in a situation where we're in a mixed cluster with a version that doesn't understand sigterm but given how nasty serialization problems are, better safe than sorry.
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.
Added.
…search into srvless-sigterm-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.
LGTM, thanks!
| Updated to |
Adds the SIGTERM shutdown type which will be used for graceful shutdown in response to SIGTERM. Right now it's the same as REPLACE.