Test behaviour depends on JDK set/map iteration ordering which varies independently of the test seed #94946
Labels
>bug
:Core/Infra/Core
Core issues without another label
Team:Core/Infra
Meta label for core/infra team
Comments
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this issue
Mar 31, 2023
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
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this issue
Mar 31, 2023
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
elasticsearchmachine
pushed a commit
that referenced
this issue
Mar 31, 2023
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
elasticsearchmachine
pushed a commit
that referenced
this issue
Mar 31, 2023
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
DaveCTurner
changed the title
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this issue
Apr 5, 2023
A handful of small changes to make the logging output of `CoordinatorTests` even more deterministic, for easier diffing. Relates elastic#94946
DaveCTurner
added a commit
that referenced
this issue
Apr 5, 2023
A handful of small changes to make the logging output of `CoordinatorTests` even more deterministic, for easier diffing. Relates #94946
Note that we've been trying (for a long time) to update to hppc (see #84168 and #109006). That update effectively does the same thing that the JDK's immutable map does, it breaks iteration order (at least, I think that is what was causing test failures, it's been a long time since I looked). |
|
|
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this issue
Jun 1, 2024
ImmutableCollections uses a seed, set early during JVM startup, which affects the iteration order of collections. Although we do not want to rely on the iteration order of Map and Set collections, bugs do sometimes occur. In order to reproduce those bugs to fix them, it is important the test seed for Elasticsearch matches the seed used in ImmutableCollections. Unfortunately ImmutableCollections is internal to the JDK, and the seed used is private and final. This commit works around these limitations by creating a patched version of ImmutableCollections which allows access to the seed member. ESTestCase is then able to reflectively set the seed at runtime based on the Elasticsearch seed. Note that this only affects tests. ImmutableCollections remains is unchanged for production code. relates elastic#94946
rjernst
added a commit
that referenced
this issue
Jun 5, 2024
ImmutableCollections uses a seed, set early during JVM startup, which affects the iteration order of collections. Although we do not want to rely on the iteration order of Map and Set collections, bugs do sometimes occur. In order to reproduce those bugs to fix them, it is important the test seed for Elasticsearch matches the seed used in ImmutableCollections. Unfortunately ImmutableCollections is internal to the JDK, and the seed used is private and final. This commit works around these limitations by creating a patched version of ImmutableCollections which allows access to the seed member. ESTestCase is then able to reflectively set the seed at runtime based on the Elasticsearch seed. Note that this only affects tests. ImmutableCollections remains is unchanged for production code. relates #94946
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Various of our test suites (most notably
CoordinatorTests) have behaviour which changes depending on the order in which we iterate through a set or a map. Since we moved from HPPC's collections to JDK's immutableSetandMapimplementations we have lost the ability to make this iteration order depend on the test seed. That means that if one of these tests fails we cannot reliably reproduce the failure (e.g. after adding more logging).Note that it is not that we want to fix the iteration order in any way: all orders are valid test cases. It's just that we want to be able to iterate the same collection in the same order in a different invocation of the JVM so that we can investigate the behaviour that led to a test failure.
See also https://mail.openjdk.org/pipermail/core-libs-dev/2023-March/102865.html.
(I'm opening this issue mainly so I have somewhere to link some related work)
94946in their vicinity):org.elasticsearch.cluster.node.DiscoveryNodes#MASTERS_FIRST_COMPARATORorg.elasticsearch.cluster.NodeConnectionsService#connectToNodesThe text was updated successfully, but these errors were encountered: