-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Make DiscoveryNodes#mastersFirstStream
deterministic
#94948
Make DiscoveryNodes#mastersFirstStream
deterministic
#94948
Conversation
Today the `CoordinatorTests` test suite is not totally deterministic because its behaviour depends on the iteration order of the JDK's unordered collections which are not under the control of our test seed. This commit makes `DiscoveryNodes#mastersFirstStream` yield the nodes in a deterministic order, which addresses one such area of unreproducibility. It's an ugly hack to do some extra work in production just for the sake of tests, but we're only sorting at most a few hundred elements here so it's not a huge deal. Relates elastic#94946
Pinging @elastic/es-distributed (Team:Distributed) |
Today the `CoordinatorTests` test suite is not totally deterministic because its behaviour depends on the iteration order of the JDK's unordered collections which are not under the control of our test seed. This commit makes `NodeConnectionsService#connectToNodes` iterate through the nodes using `DiscoveryNodes#mastersFirstStream`, which is made deterministic by elastic#94948. It's an ugly hack to do some extra work in production just for the sake of tests, but we're only sorting at most a few hundred elements here so it's not a huge deal. Relates elastic#94946
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.
LGTM with one small comment!
@@ -185,11 +186,15 @@ public Map<String, DiscoveryNode> getCoordinatingOnlyNodes() { | |||
return filteredNodes(nodes, n -> n.canContainData() == false && n.isMasterNode() == false && n.isIngestNode() == false); | |||
} | |||
|
|||
// Ugly hack: when https://github.com/elastic/elasticsearch/issues/94946 is fixed, remove the sorting by ephemeral ID here | |||
private final Comparator<DiscoveryNode> MASTERS_FIRST_COMPARATOR = Comparator.<DiscoveryNode>comparingInt(n -> n.isMasterNode() ? 0 : 1) |
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 believe static
is missed here since the comparator is stateless
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.
Ah oops yes I meant static
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.
LGTM
Today the `CoordinatorTests` test suite is not totally deterministic because its behaviour depends on the iteration order of the JDK's unordered collections which are not under the control of our test seed. This commit makes `NodeConnectionsService#connectToNodes` iterate through the nodes using `DiscoveryNodes#mastersFirstStream`, which is made deterministic by #94948. It's an ugly hack to do some extra work in production just for the sake of tests, but we're only sorting at most a few hundred elements here so it's not a huge deal. Relates #94946
Today the `CoordinatorTests` test suite is not totally deterministic because its behaviour depends on the iteration order of the JDK's unordered collections which are not under the control of our test seed. This commit makes `DiscoveryNodes#mastersFirstStream` yield the nodes in a deterministic order, which addresses one such area of unreproducibility. It's an ugly hack to do some extra work in production just for the sake of tests, but we're only sorting at most a few hundred elements here so it's not a huge deal. Relates elastic#94946
…4949) Today the `CoordinatorTests` test suite is not totally deterministic because its behaviour depends on the iteration order of the JDK's unordered collections which are not under the control of our test seed. This commit makes `NodeConnectionsService#connectToNodes` iterate through the nodes using `DiscoveryNodes#mastersFirstStream`, which is made deterministic by elastic#94948. It's an ugly hack to do some extra work in production just for the sake of tests, but we're only sorting at most a few hundred elements here so it's not a huge deal. Relates elastic#94946
Today the `CoordinatorTests` test suite is not totally deterministic because its behaviour depends on the iteration order of the JDK's unordered collections which are not under the control of our test seed. This commit makes `DiscoveryNodes#mastersFirstStream` yield the nodes in a deterministic order, which addresses one such area of unreproducibility. It's an ugly hack to do some extra work in production just for the sake of tests, but we're only sorting at most a few hundred elements here so it's not a huge deal. Relates elastic#94946
…4949) Today the `CoordinatorTests` test suite is not totally deterministic because its behaviour depends on the iteration order of the JDK's unordered collections which are not under the control of our test seed. This commit makes `NodeConnectionsService#connectToNodes` iterate through the nodes using `DiscoveryNodes#mastersFirstStream`, which is made deterministic by elastic#94948. It's an ugly hack to do some extra work in production just for the sake of tests, but we're only sorting at most a few hundred elements here so it's not a huge deal. Relates elastic#94946
Today the
CoordinatorTests
test suite is not totally deterministic because its behaviour depends on the iteration order of the JDK's unordered collections which are not under the control of our test seed.This commit makes
DiscoveryNodes#mastersFirstStream
yield the nodes in a deterministic order, which addresses one such area of unreproducibility. It's an ugly hack to do some extra work in production just for the sake of tests, but we're only sorting at most a few hundred elements here so it's not a huge deal.Relates #94946