Skip to content

Zen2: Move most integration tests to Zen2 #35678

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 19, 2018
Merged

Zen2: Move most integration tests to Zen2 #35678

merged 5 commits into from
Nov 19, 2018

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Nov 18, 2018 •
edited
Loading

Zen2 is now feature-complete enough to run most ESIntegTestCase tests. The changes in this PR are as follows:

  • ClusterSettingsIT is adapted to not be Zen1 specific anymore (it was using Zen1 settings).
  • Some of the integration tests require persistent storage of the cluster state, which is not fully implemented yet (see Zen2 ClusterState storage #33958). These tests keep running with Zen1 for now but will be switched over as soon as that is fully implemented.
  • Some very few integration tests are not running yet with Zen2 for other reasons, depending on some of the other open points in A new cluster coordination layer #32006.

@ywelsch ywelsch added >non-issue v7.0.0 :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Nov 18, 2018
@ywelsch ywelsch requested a review from DaveCTurner November 18, 2018 10:26
@ywelsch ywelsch mentioned this pull request Nov 18, 2018
Closed
61 tasks
DaveCTurner
DaveCTurner reviewed Nov 19, 2018
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I left a few comments.

server/src/test/java/org/elasticsearch/cluster/settings/ClusterSettingsIT.java Outdated
assertEquals(discoverySettings.getPublishTimeout(), DiscoverySettings.PUBLISH_TIMEOUT_SETTING.get(Settings.EMPTY));
assertTrue(DiscoverySettings.PUBLISH_DIFF_ENABLE_SETTING.get(Settings.EMPTY));
final Setting<Integer> INITIAL_RECOVERIES = CLUSTER_ROUTING_ALLOCATION_NODE_INITIAL_PRIMARIES_RECOVERIES_SETTING;
final Setting<Integer> CONCURRENT_RECOVIERS = CLUSTER_ROUTING_ALLOCATION_NODE_CONCURRENT_RECOVERIES_SETTING;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: RECOVIERS -> RECOVERIES

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 164dd3a

server/src/test/java/org/elasticsearch/cluster/settings/ClusterSettingsIT.java Outdated
.put(DiscoverySettings.PUBLISH_DIFF_ENABLE_SETTING.getKey(), false)
.put(DiscoverySettings.PUBLISH_TIMEOUT_SETTING.getKey(), "1s").build())
.put(INITIAL_RECOVERIES.getKey(), 7)
.put(CONCURRENT_RECOVIERS.getKey(), 42).build())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should maintain the variety in the types a bit - we used to have a boolean and a time-value but now there's just integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 7f590ea

server/src/test/java/org/elasticsearch/indices/state/RareClusterStateIT.java Outdated
@@ -192,6 +192,7 @@ public void testDelayedMappingPropagationOnPrimary() throws Exception {
Settings settings = Settings.builder()
.put(DiscoverySettings.COMMIT_TIMEOUT_SETTING.getKey(), "30s") // explicitly set so it won't default to publish timeout
.put(DiscoverySettings.PUBLISH_TIMEOUT_SETTING.getKey(), "0s") // don't wait post commit as we are blocking things by design
.put(TestZenDiscovery.USE_ZEN2.getKey(), false) // stops 1 node out of 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stopping one node from two should be ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the explanation was bad here, the node is not stopped, but partitioned away. The gist is that this tests needs bigger changes to work with Zen2. Nothing particularly wrong with Zen2, just that this test requires a very peculiar setup. I have not made my mind up yet how to refactor it, so I will keep this as TODO for now. I've pushed 36b5cde to change the comment.

server/src/test/java/org/elasticsearch/indices/state/RareClusterStateIT.java Outdated
@@ -306,6 +307,7 @@ public void testDelayedMappingPropagationOnReplica() throws Exception {
Settings.builder()
.put(DiscoverySettings.COMMIT_TIMEOUT_SETTING.getKey(), "30s") // explicitly set so it won't default to publish timeout
.put(DiscoverySettings.PUBLISH_TIMEOUT_SETTING.getKey(), "0s") // don't wait post commit as we are blocking things by design
.put(TestZenDiscovery.USE_ZEN2.getKey(), false) // stops 1 node out of 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stopping one node from two should be ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

...t/java/org/elasticsearch/xpack/security/transport/ServerTransportFilterIntegrationTests.java
@@ -91,7 +91,7 @@ public void testThatConnectionToServerTypeConnectionWorks() throws IOException,
Path xpackConf = home.resolve("config");
Files.createDirectories(xpackConf);

Transport transport = internalCluster().getDataNodeInstance(Transport.class);
Transport transport = internalCluster().getMasterNodeInstance(Transport.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we intend this change in behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, Zen2 requires for there to be a master-eligible node in the unicast hosts list in order to find and rejoin the existing cluster.

test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java Outdated
private static final class MasterNodePredicate implements Predicate<NodeAndClient> {
private final String masterNodeName;
private final Optional<String> masterNodeName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I preferred the DataOrMasterNodePredicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed further on the refactoring (see aa37d57). Let me know if you like this more.

@ywelsch ywelsch requested a review from DaveCTurner November 19, 2018 15:59
DaveCTurner
DaveCTurner approved these changes Nov 19, 2018
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ywelsch ywelsch merged commit 47ada69 into elastic:zen2 Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees
No one assigned
Labels
:Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >non-issue v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ywelsch @DaveCTurner @colings86