Skip to content

Conversation

@stu-elastic
Copy link
Contributor

@stu-elastic stu-elastic commented Apr 20, 2023

Adds the SIGTERM shutdown type which will be used for graceful shutdown in response to SIGTERM. Right now it's the same as REPLACE.

@stu-elastic stu-elastic marked this pull request as ready for review April 24, 2023 16:44
@stu-elastic stu-elastic added :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown and removed WIP labels Apr 24, 2023
@stu-elastic stu-elastic requested a review from gwbrown April 24, 2023 16:44
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Apr 24, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

Hi @stu-elastic, I've created a changelog YAML for you.

Copy link
Contributor

@gwbrown gwbrown left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SIGTERM; // local version of REMOVE
SIGTERM; // locally-initiated version of REMOVE
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment on lines 79 to 81
singleNodeShutdown -> singleNodeShutdown.getType() == SingleNodeShutdownMetadata.Type.REMOVE
|| singleNodeShutdown.getType() == SingleNodeShutdownMetadata.Type.SIGTERM
|| singleNodeShutdown.getType() == SingleNodeShutdownMetadata.Type.REPLACE
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@gmarouli gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023
@stu-elastic
Copy link
Contributor Author

Updated to TransportVersion.V_8_9_0

@stu-elastic stu-elastic merged commit 41575d7 into elastic:main Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown >enhancement Team:Core/Infra Meta label for core/infra team v8.9.0

4 participants