-
Notifications
You must be signed in to change notification settings - Fork 25.2k
[Zen2] Remove initial master node count setting #37150
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
[Zen2] Remove initial master node count setting #37150
Conversation
Pinging @elastic/es-distributed |
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.
Thanks for picking this up. I've left mostly minor comments.
...org/elasticsearch/action/admin/cluster/bootstrap/TransportGetDiscoveredNodesActionTests.java
Outdated
Show resolved
Hide resolved
...org/elasticsearch/action/admin/cluster/bootstrap/TransportGetDiscoveredNodesActionTests.java
Outdated
Show resolved
Hide resolved
internalCluster().setDisruptionScheme(disruptionScheme); | ||
disruptionScheme.startDisrupting(); | ||
|
||
final Client remainingClient = client(); |
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.
I'm confused. Why capture the client here and not just use client()
at usage sites below?
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.
We first check that we have a client to a node that has applied the no-master block. The alternative would be to wait for all nodes to apply the no-master block before proceeding.
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.
However the name was a hangover from the earlier implementation that shut nodes down, so I renamed it in 788df4a.
test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java
Outdated
Show resolved
Hide resolved
test/framework/src/test/java/org/elasticsearch/test/test/InternalTestClusterTests.java
Outdated
Show resolved
Hide resolved
if (autoManageMinMasterNodes) { | ||
return allNodesSettings; | ||
} | ||
return addBootstrapConfiguration(new Random(bootstrapNodeSelectionSeed), allNodesSettings); |
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.
why not just random()
here?
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.
We run this twice with the same seed to verify that the behaviour is deterministic, but that seed does not affect the randomness supplied by random()
.
@elasticmachine please run the Gradle build tests 1 |
The
cluster.unsafe_initial_master_node_count
setting was introduced as atemporary measure while the design of
cluster.initial_master_nodes
was beingfinalised. This commit removes this temporary setting, replacing it with usages
of
cluster.initial_master_nodes
where appropriate.