-
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 NodeConnectionsService#connectToNodes
deterministic
#94949
Make NodeConnectionsService#connectToNodes
deterministic
#94949
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 `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
Pinging @elastic/es-distributed (Team: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.
LGTM
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! I've left a small suggestion
@@ -99,7 +100,9 @@ public void connectToNodes(DiscoveryNodes discoveryNodes, Runnable onCompletion) | |||
final List<Runnable> runnables = new ArrayList<>(discoveryNodes.getSize()); | |||
try (var refs = new RefCountingRunnable(onCompletion)) { | |||
synchronized (mutex) { | |||
for (final DiscoveryNode discoveryNode : discoveryNodes) { | |||
// Ugly hack: when https://github.com/elastic/elasticsearch/issues/94946 is fixed, just iterate over discoveryNodes here | |||
for (final Iterator<DiscoveryNode> iterator = discoveryNodes.mastersFirstStream().iterator(); iterator.hasNext();) { |
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.
If we are going to use this pattern in multiple places, what do you think about creating a helper method that yields an iterable object that represents master first sorted discoveryNodes?
public Iterable<DiscoveryNode> mastersFirst() {
return mastersFirstStream()::iterator;
}
Then we can continue using for loops as
for (final DiscoveryNode discoveryNode : discoveryNodes.mastersFirst()) {
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, yes, that'd make sense. I think it's just here for now so I'll leave it as it is to avoid adding too much more cruft to the DiscoveryNodes
API, but I will do this if I encounter any more of these.
@elasticmachine update branch |
@elasticmachine please run elasticsearch-ci/part-2 |
…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
…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
NodeConnectionsService#connectToNodes
iterate through the nodes usingDiscoveryNodes#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