Skip to content

Ack node shutdown requests after cluster state update is complete#85846

Merged
rjernst merged 4 commits intoelastic:masterfrom
rjernst:shutdown/ack
Apr 13, 2022
Merged

Ack node shutdown requests after cluster state update is complete#85846
rjernst merged 4 commits intoelastic:masterfrom
rjernst:shutdown/ack

Conversation

@rjernst
Copy link
Copy Markdown
Member

@rjernst rjernst commented Apr 12, 2022

The adding or removing a node to the node shutdown api stores submits a
cluster state update task to store the node metadata to be shutdown.
Sometimes a reroute is required, and in those cases we currently wait
for the reroute to complete before acknowledging the original request.
This commit changes the behavior to acknowledge success after the
cluster state update is complete. A warning is still logged if the
reroute fails.

relates #84847

The adding or removing a node to the node shutdown api stores submits a
cluster state update task to store the node metadata to be shutdown.
Sometimes a reroute is required, and in those cases we currently wait
for the reroute to complete before acknowledging the original request.
This commit changes the behavior to acknowledge success after the
cluster state update is complete. A warning is still logged if the
reroute fails.

relates elastic#84847
@rjernst rjernst added >refactoring :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown v8.3.0 labels Apr 12, 2022
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Apr 12, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

@rjernst rjernst requested a review from pgomulka April 12, 2022 21:34

@Override
public void clusterStateProcessed(ClusterState oldState, ClusterState newState) {
listener.onResponse(AcknowledgedResponse.TRUE);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest completing the listener after submitting the task to the reroute service, so that if there's ever a need to wait for the followup reroute to complete you can safely do so with GET _cluster/health?wait_for_events=languid.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest completing the listener after submitting the task to the reroute service, so that if there's ever a need to wait for the followup reroute to complete you can safely do so with GET _cluster/health?wait_for_events=languid.

(sorry for the noise, repeating my previous comment in a new review)

Copy link
Copy Markdown
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM,

Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Sorry, forgot to mark my previous review as request changes, resubmitting...


@Override
public void clusterStateProcessed(ClusterState oldState, ClusterState newState) {
listener.onResponse(AcknowledgedResponse.TRUE);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest completing the listener after submitting the task to the reroute service, so that if there's ever a need to wait for the followup reroute to complete you can safely do so with GET _cluster/health?wait_for_events=languid.

(sorry for the noise, repeating my previous comment in a new review)

Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@rjernst rjernst merged commit ee101b4 into elastic:master Apr 13, 2022
@rjernst rjernst deleted the shutdown/ack branch April 13, 2022 21:35
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 >refactoring Team:Core/Infra Meta label for core/infra team v8.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants