Skip to content

Convert testclusters to use distro download plugin #44253

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

Merged
merged 7 commits into from
Jul 15, 2019
Merged

Convert testclusters to use distro download plugin #44253

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
47407aa
Convert testclusters to use distro download plugin
rjernst Jul 12, 2019
16584eb
Merge branch 'master' into distro_tests13
rjernst Jul 12, 2019
6d897bd
address feedback, move default distro and version into node
rjernst Jul 13, 2019
3b2ff7d
use setters for setting defaults
rjernst Jul 13, 2019
95d0b1a
solidify naming of test distro enum
rjernst Jul 14, 2019
c28dbbf
fix third party audit tests
rjernst Jul 14, 2019
870e109
Merge branch 'master' into distro_tests13
rjernst Jul 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 0 additions & 80 deletions buildSrc/src/main/java/org/elasticsearch/gradle/Distribution.java
View file
Open in desktop

This file was deleted.

7 changes: 7 additions & 0 deletions buildSrc/src/main/java/org/elasticsearch/gradle/ElasticsearchDistribution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@
package org.elasticsearch.gradle;

import org.gradle.api.Buildable;
import org.gradle.api.Project;
import org.gradle.api.artifacts.Configuration;
import org.gradle.api.file.FileTree;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.provider.Property;
import org.gradle.api.tasks.TaskDependency;

import java.io.File;
import java.util.Iterator;
import java.util.Locale;
import java.util.concurrent.Callable;

public class ElasticsearchDistribution implements Buildable {

Expand Down Expand Up @@ -90,6 +93,10 @@ public TaskDependency getBuildDependencies() {
return configuration.getBuildDependencies();
}

public FileTree getFileTree(Project project) {
return project.fileTree((Callable<File>) configuration::getSingleFile);
}

@Override
public String toString() {
return configuration.getSingleFile().toString();
Expand Down
4 changes: 2 additions & 2 deletions buildSrc/src/main/java/org/elasticsearch/gradle/test/RestTestRunnerTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import java.util.ArrayList;
import java.util.Collection;

import static org.elasticsearch.gradle.Distribution.INTEG_TEST;
import static org.elasticsearch.gradle.testclusters.TestDistribution.INTEG_TEST;

/**
* Customized version of Gradle {@link Test} task which tracks a collection of {@link ElasticsearchCluster} as a task input. We must do this
Expand All @@ -23,7 +23,7 @@ public class RestTestRunnerTask extends Test {
public RestTestRunnerTask() {
super();
this.getOutputs().doNotCacheIf("Build cache is only enabled for tests against clusters using the 'integ-test' distribution",
task -> clusters.stream().flatMap(c -> c.getNodes().stream()).anyMatch(n -> n.getDistribution() != INTEG_TEST));
task -> clusters.stream().flatMap(c -> c.getNodes().stream()).anyMatch(n -> n.getTestDistribution() != INTEG_TEST));
}

@Nested
Expand Down
25 changes: 13 additions & 12 deletions buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchCluster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/
package org.elasticsearch.gradle.testclusters;

import org.elasticsearch.gradle.Distribution;
import org.elasticsearch.gradle.ElasticsearchDistribution;
import org.elasticsearch.gradle.FileSupplier;
import org.elasticsearch.gradle.PropertyNormalization;
import org.elasticsearch.gradle.Version;
Expand Down Expand Up @@ -60,22 +60,23 @@ public class ElasticsearchCluster implements TestClusterConfiguration, Named {
private final String clusterName;
private final NamedDomainObjectContainer<ElasticsearchNode> nodes;
private final File workingDirBase;
private final File artifactsExtractDir;
private final Function<Integer, ElasticsearchDistribution> distributionFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

I added a comment here previously which seems to have vanished.

It seems that we are generating a distribution per node, defeating the purpose of syncWithLinks.
This optimization is important both in terms of disk space and time the build takes and I would prefer we don't merge this without it.

It might make more sense to have this optimization in the distribution downloaded plugin trough and have test cluusters use the extracted distro dirs directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

The distribution download plugin already optimizes by only doing a single extraction for a given set of distribution properties. While we create a unique ElasticsearchDistribution object per node here, the underlying extracted dir will just exist once, in the root project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, got it, we only use the index because we need unique names for the domain object container.
I wonder if we should be concerned about name clashes here ? In theory other uses of the distribution download plugin could use the same name as testclusters even trough unlikely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clashes are possible but will result in an obvious error. I don't think we need to be too concerned about it.

private final LinkedHashMap<String, Predicate<TestClusterConfiguration>> waitConditions = new LinkedHashMap<>();
private final Project project;

public ElasticsearchCluster(String path, String clusterName, Project project, File artifactsExtractDir, File workingDirBase) {
public ElasticsearchCluster(String path, String clusterName, Project project,
Function<Integer, ElasticsearchDistribution> distributionFactory, File workingDirBase) {
this.path = path;
this.clusterName = clusterName;
this.project = project;
this.distributionFactory = distributionFactory;
this.workingDirBase = workingDirBase;
this.artifactsExtractDir = artifactsExtractDir;
this.nodes = project.container(ElasticsearchNode.class);
this.nodes.add(
new ElasticsearchNode(
path, clusterName + "-0",
project, artifactsExtractDir, workingDirBase
)
project, workingDirBase, distributionFactory.apply(0)
)
);
// configure the cluster name eagerly so nodes know about it
this.nodes.all((node) -> node.defaultConfig.put("cluster.name", safeName(clusterName)));
Expand All @@ -98,8 +99,8 @@ public void setNumberOfNodes(int numberOfNodes) {

for (int i = nodes.size() ; i < numberOfNodes; i++) {
this.nodes.add(new ElasticsearchNode(
path, clusterName + "-" + i, project, artifactsExtractDir, workingDirBase
));
path, clusterName + "-" + i, project, workingDirBase, distributionFactory.apply(i)
));
}
}

Expand All @@ -121,7 +122,7 @@ public void setVersion(String version) {
}

@Override
public void setDistribution(Distribution distribution) {
public void setDistribution(TestDistribution distribution) {
nodes.all(each -> each.setDistribution(distribution));
}

Expand Down Expand Up @@ -248,7 +249,7 @@ public void start() {
for (ElasticsearchNode node : nodes) {
if (nodeNames != null) {
// Can only configure master nodes if we have node names defined
if (Version.fromString(node.getVersion()).getMajor() >= 7) {
if (node.getVersion().getMajor() >= 7) {
node.defaultConfig.put("cluster.initial_master_nodes", "[" + nodeNames + "]");
node.defaultConfig.put("discovery.seed_providers", "file");
node.defaultConfig.put("discovery.seed_hosts", "[]");
Expand Down Expand Up @@ -338,9 +339,9 @@ public boolean isProcessAlive() {
return nodes.stream().noneMatch(node -> node.isProcessAlive() == false);
}

void eachVersionedDistribution(BiConsumer<String, Distribution> consumer) {
void eachVersionedDistribution(BiConsumer<Version, TestDistribution> consumer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not 100% sure but this might no longer be used now

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. This can now be removed.

nodes.forEach(each -> {
consumer.accept(each.getVersion(), each.getDistribution());
consumer.accept(each.getVersion(), each.getTestDistribution());
});
}

Expand Down
Loading