-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Testclusters: implement starting, waiting for and stopping single cluster nodes #35599
Conversation
Pinging @elastic/es-core-infra |
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Show resolved
Hide resolved
Thanks for the review @original-brownbear ! I left some answers. |
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Outdated
Show resolved
Hide resolved
@atorok thanks! Looks good in general :), but see my comment about the thread leak. |
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Show resolved
Hide resolved
buildSrc/src/test/java/org/elasticsearch/gradle/test/GradleIntegrationTestCase.java
Show resolved
Hide resolved
I moved the cleanup of processes outside of |
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersPlugin.java
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersPlugin.java
Outdated
Show resolved
Hide resolved
@nik9000 can you take another look please. |
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.
I'm happy with this. I think it pushes the project forwards and i like it. I think there are a couple of unrelated changes bundled up in it. At least, I can't see how they are related. Could you explain them? Or maybe move them into their own PR so they get a nice commit message explaining them?
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/test/NamingConventionsCheck.java
Outdated
Show resolved
Hide resolved
@nik9000 I should have offered more details on these changes . I now realize they seem out of place. They do need to be part of this PR. |
@nik9000 we do also need to run the naming convention check with the compiler java as build-tools how has classes for newer java versions. We could do it locally for build tools only but it doesn't matter much since we'll be using compiler java for this check with |
@elasticmachine run gradle tests 1 |
@elasticmachine run gradle build tests 1 |
@nik9000 I double checked to be sure. All comments on the PR should be addressed. |
@elasticmachine run default distro tests |
1 similar comment
@elasticmachine run default distro tests |
The bulk of the request is new functionality in
ElasticsearchNode
.Note that this offers 0 configuration, this ability will be added as we start to use testclsuters and find a need for things to be configurable to keep these to a minimum.
My thinking is that we can auto configure in some of the cases where we have configuration for cluster-formation ( e.x. detect that security is installed and do the right checks automatically).
The accompanying tests are still the only users of this functionality.
A quick map to help navigate the PR:
writeConfigurationFile
ProcessBuilder
and redirect the streams to filesstart
registerCleanupHooks
stop
and reads the files into the stream on failure.Tests are passing on Windows.