Skip to content

Simplify node stopping call #88547

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 22, 2022
Merged

Simplify node stopping call #88547

merged 3 commits into from
Jul 22, 2022

Conversation

idegtiarenko
Copy link
Contributor

There is no need to use randomNode/predicate combination to stop a node by its
name.

@idegtiarenko
Simplify node stopping call
2b7a987 
There is no need to use randomNode/predicate combination to stop a node by its
name.
@idegtiarenko idegtiarenko added >non-issue :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.4.0 labels Jul 14, 2022
@idegtiarenko idegtiarenko requested a review from DaveCTurner July 14, 2022 13:03
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

DaveCTurner
DaveCTurner reviewed Jul 19, 2022
View reviewed changes
Copy link
Contributor

@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.

Nicely spotted.

However this loses the check that we're not stopping the last remaining master-eligible node. Could we keep that?

I wonder if we can get rid of stopRandomNode entirely, replacing it with a function that selects a random node's name according to the given settings predicate?

@idegtiarenko
Copy link
Contributor Author

However this loses the check that we're not stopping the last remaining master-eligible node. Could we keep that?

Added the check to existing stopNode(String nodeName).

I wonder if we can get rid of stopRandomNode entirely, replacing it with a function that selects a random node's name according to the given settings predicate?

Would you mind if I do it in a separate pr to make this change smaller?

@idegtiarenko idegtiarenko requested a review from DaveCTurner July 21, 2022 13:53
DaveCTurner
DaveCTurner approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@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

@idegtiarenko
Copy link
Contributor Author

@elasticmachine please run elasticsearch-ci/part-1

@idegtiarenko idegtiarenko merged commit 584f7f2 into elastic:master Jul 22, 2022
@idegtiarenko idegtiarenko deleted the siplify_tests branch July 22, 2022 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees
No one assigned
Labels
:Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@idegtiarenko @elasticmachine @DaveCTurner