Skip to content
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

Nodetool Drain throws immediately if the server is not initialized #584

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

rhuffy
Copy link
Contributor

@rhuffy rhuffy commented Nov 21, 2024

Since the drain method is synchronized, calls to nodetool drain will hang if the server is not initialized. This includes nodes that are bootstrapping.

This PR makes these calls to drain() fail fast, so the behavior is more obvious.

Comment on lines 4615 to 4618
else
{
throw new IllegalStateException("Cannot drain a node that is bootstrapping");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this would happen if we called drain during regular startup as well, not just bootstrapping right

Copy link
Contributor

Choose a reason for hiding this comment

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

then there's a question: if we're starting up vs bootstrapping is it desirable to wait for startup to complete then drain, or just fail the drain and continue w/ other shutdown actions later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only case we should expect Drain to be called when the server is not fully initialized is if someone is taking manual action to terminate a node that is stuck initializing, or taking manual action to abort an in-progress bootstrap or node replacement. These are probably both cases we'd want to terminate without delay.

Copy link
Contributor

@zpear zpear left a comment

Choose a reason for hiding this comment

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

Lgtm

@rhuffy rhuffy merged commit 47abc3b into palantir-cassandra-2.2.18 Nov 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants