Skip to content

Commit

Permalink
Make DiscoveryNodes#mastersFirstStream deterministic (elastic#94948)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
DaveCTurner authored and rjernst committed Apr 6, 2023
1 parent d58b034 commit 3494a83
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.AbstractCollection;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -185,11 +186,15 @@ public Map<String, DiscoveryNode> getCoordinatingOnlyNodes() {
return filteredNodes(nodes, n -> n.canContainData() == false && n.isMasterNode() == false && n.isIngestNode() == false);
}

private static final Comparator<DiscoveryNode> MASTERS_FIRST_COMPARATOR
// Ugly hack: when https://github.com/elastic/elasticsearch/issues/94946 is fixed, remove the sorting by ephemeral ID here
= Comparator.<DiscoveryNode>comparingInt(n -> n.isMasterNode() ? 0 : 1).thenComparing(DiscoveryNode::getEphemeralId);

/**
* Returns a stream of all nodes, with master nodes at the front
*/
public Stream<DiscoveryNode> mastersFirstStream() {
return Stream.concat(masterNodes.values().stream(), stream().filter(n -> n.isMasterNode() == false));
return nodes.values().stream().sorted(MASTERS_FIRST_COMPARATOR);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -160,9 +159,16 @@ public void testMastersFirst() {
final List<DiscoveryNode> returnedNodes = discoBuilder.build().mastersFirstStream().toList();
assertEquals(returnedNodes.size(), inputNodes.size());
assertEquals(new HashSet<>(returnedNodes), new HashSet<>(inputNodes));
final List<DiscoveryNode> sortedNodes = new ArrayList<>(returnedNodes);
Collections.sort(sortedNodes, Comparator.comparing(n -> n.isMasterNode() == false));
assertEquals(sortedNodes, returnedNodes);

boolean mastersOk = true;
final var message = returnedNodes.toString();
for (final var discoveryNode : returnedNodes) {
if (discoveryNode.isMasterNode()) {
assertTrue(message, mastersOk);
} else {
mastersOk = false;
}
}
}

public void testDeltaListsMultipleNodes() {
Expand Down

0 comments on commit 3494a83

Please sign in to comment.