Skip to content

Remove random when using HLRC sync and async calls #48211

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 5 commits into from
Oct 24, 2019
Merged

Remove random when using HLRC sync and async calls #48211

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8122506
Remove random when using HLRC sync and async calls
hub-cap Oct 17, 2019
312c2ce
Move the async envvar read
hub-cap Oct 18, 2019
709e622
Merge remote-tracking branch 'upstream/master' into hlrc_remove_random
hub-cap Oct 22, 2019
2ac6686
Merge branch 'master' of github.com:elastic/elasticsearch into hlrc_r…
hub-cap Oct 23, 2019
ec524bd
Fix a test failure
hub-cap Oct 23, 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
68 changes: 43 additions & 25 deletions client/rest-high-level/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import org.elasticsearch.gradle.test.RestIntegTestTask

/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
Expand Down Expand Up @@ -106,33 +108,49 @@ File nodeTrustStore = file("./testnode.jks")
File pkiTrustCert = file("./src/test/resources/org/elasticsearch/client/security/delegate_pki/testRootCA.crt")

integTest.runner {
systemProperty 'tests.rest.async', 'false'
systemProperty 'tests.rest.cluster.username', System.getProperty('tests.rest.cluster.username', 'test_user')
systemProperty 'tests.rest.cluster.password', System.getProperty('tests.rest.cluster.password', 'test-password')
}

testClusters.integTest {
testDistribution = 'DEFAULT'
systemProperty 'es.scripting.update.ctx_in_params', 'false'
setting 'reindex.remote.whitelist', '[ "[::1]:*", "127.0.0.1:*" ]'
setting 'xpack.license.self_generated.type', 'trial'
setting 'xpack.security.enabled', 'true'
setting 'xpack.security.authc.token.enabled', 'true'
setting 'xpack.security.authc.api_key.enabled', 'true'
// Truststore settings are not used since TLS is not enabled. Included for testing the get certificates API
setting 'xpack.security.http.ssl.certificate_authorities', 'testnode.crt'
setting 'xpack.security.transport.ssl.truststore.path', 'testnode.jks'
setting 'xpack.security.authc.realms.file.default_file.order', '0'
setting 'xpack.security.authc.realms.native.default_native.order', '1'
setting 'xpack.security.authc.realms.pki.pki1.order', '2'
setting 'xpack.security.authc.realms.pki.pki1.certificate_authorities', '[ "testRootCA.crt" ]'
setting 'xpack.security.authc.realms.pki.pki1.delegation.enabled', 'true'

setting 'indices.lifecycle.poll_interval', '1000ms'
keystore 'xpack.security.transport.ssl.truststore.secure_password', 'testnode'
user username: System.getProperty('tests.rest.cluster.username', 'test_user'),
password: System.getProperty('tests.rest.cluster.password', 'test-password')

extraConfigFile nodeCert.name, nodeCert
extraConfigFile nodeTrustStore.name, nodeTrustStore
extraConfigFile pkiTrustCert.name, pkiTrustCert
// TODO: we can't use task avoidance here because RestIntegTestTask does the testcluster creation
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of RestTestRunnerTask would be preferable here.
You can have two tasks use the same cluster ( even trough right now it disabled the build cache ).
Just need to add useCluster testClusters.integTest to the task, and it can use task avoidance too.
No need for the asyncIntegTest as it won't really be used at the same time.
Assuming cleanup works as it should, we should be able to run the tests against the same cluster.

Another approach is to add sub-projects so the tests can run in parallel.
The downside is that we don't have a generic solution to wire the tests up to the sub-projects ( even trough we have examples elsewhere in the code )

@mark-vieira might also have an opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Id love some better advice here, especially if it does not double the build time for this heh

Copy link
Contributor

Choose a reason for hiding this comment

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

If time is the primary concern, then sub-projects are the way to go.
That will matter when running a small number of tasks, like running the tests for the client only so there are resources to spare on the machine, it won't matter as much for large runs that run everything in CI where there are plenty of projects to compete for the available resources.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should do subprojects until we have a simple and clear way to set this up via a gradle plugin. There is complexity involved in properly transfering the test classes via a jar, extracting them, then convincing the test runner in the subproject to use the extracted class files. Another task here is fine (and I think another cluster is fine too, we don't need to share, since we link the main directories there is minimal that is copied, and the setup here is minimal since it is not a rest test). It matches what we did for transport-netty4 with pooled/unpooled tests.

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 use two clusters and two tasks. Having multiple tasks targeting the same cluster disables caching. Having multiple clusters that are technically identical doesn't actually have much in the way of overhead due to the way we set these up with hardlinks but the benefit of being able to cache these tests is potentially significant.

I don't completely understand this comment:

TODO: we can't use task avoidance here because RestIntegTestTask does the testcluster creation

This is technically correct, creating a RestIntegTestTask creates the corresponding cluster but this doesn't affect build avoidance in any way. Everything is still wired up in a cacheable way.

Also, -1 to multiple subprojects. These test executions are identical in every way except for a single system property set on the test executor itself. There's absolutely no reason to create subprojects except for the fact that would facilitate running these suites in parallel which I don't think is very compelling given the way our builds are structured and already highly parallelizable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think that this argument is compelling. It's so unfortunate that there's no way to run test tasks in parallel for the same project!
For someone developing the client who has to run these tests often it will make a big difference as it's likely to cut the time it takes in half. I do agree it doesn't make a bug difference for larger runs in CI running tests for a large number of projects.
I 100% agree that we should have a plugin if we are to go with the sub-project route, now is as good of a time as any.
Just to be clear @hub-cap , I don't think there's anything here to prevent mergin the PR I'm just happy it triggered the discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think that this argument is compelling. It's so unfortunate that there's no way to run test tasks in parallel for the same project!

This is better solved in Gradle core by moving test executing to the worker API. I know this is something they want to do but not sure where it stacks up on the roadmap. I'm still highly annoyed that they got rid of @ParallelizableTask in Gradle 4.0 w/o a proper replacement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review friends, so from what I gather I can leave it as is (sans fixing the nits of course).

as for the comment @mark-vieira, it was copied from an earlier comment in some other code that did something similar, I just did not want to lose whatever reference to what was in the original authors head. But it sounds like I can just remove this comment in both places from your comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you are referring to this comment. I would say yes. I believe that comment predates some things we've done around RestIntegTestTask to make it reliably cacheable. In scenarios where the tests cannot be cached the build will "detect" this automatically and disable it so there's not need to explicitly document such scenarios.

+1 on removing that comment in both places.

RestIntegTestTask asyncIntegTest = tasks.create("asyncIntegTest", RestIntegTestTask) {
runner {
systemProperty 'tests.rest.async', 'true'
systemProperty 'tests.rest.cluster.username', System.getProperty('tests.rest.cluster.username', 'test_user')
systemProperty 'tests.rest.cluster.password', System.getProperty('tests.rest.cluster.password', 'test-password')
}
}

check.dependsOn(asyncIntegTest)

['integTest', 'asyncIntegTest'].each { integName ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the async system prop is for the tests not the cluster itself these clusters are identical. We can replace this with testClusters.all { // your config }.

testClusters {
"${integName}" {
testDistribution = 'DEFAULT'
systemProperty 'es.scripting.update.ctx_in_params', 'false'
setting 'reindex.remote.whitelist', '[ "[::1]:*", "127.0.0.1:*" ]'
setting 'xpack.license.self_generated.type', 'trial'
setting 'xpack.security.enabled', 'true'
setting 'xpack.security.authc.token.enabled', 'true'
setting 'xpack.security.authc.api_key.enabled', 'true'
// Truststore settings are not used since TLS is not enabled. Included for testing the get certificates API
setting 'xpack.security.http.ssl.certificate_authorities', 'testnode.crt'
setting 'xpack.security.transport.ssl.truststore.path', 'testnode.jks'
setting 'xpack.security.authc.realms.file.default_file.order', '0'
setting 'xpack.security.authc.realms.native.default_native.order', '1'
setting 'xpack.security.authc.realms.pki.pki1.order', '2'
setting 'xpack.security.authc.realms.pki.pki1.certificate_authorities', '[ "testRootCA.crt" ]'
setting 'xpack.security.authc.realms.pki.pki1.delegation.enabled', 'true'

setting 'indices.lifecycle.poll_interval', '1000ms'
keystore 'xpack.security.transport.ssl.truststore.secure_password', 'testnode'
user username: System.getProperty('tests.rest.cluster.username', 'test_user'),
password: System.getProperty('tests.rest.cluster.password', 'test-password')

extraConfigFile nodeCert.name, nodeCert
extraConfigFile nodeTrustStore.name, nodeTrustStore
extraConfigFile pkiTrustCert.name, pkiTrustCert
}
}
}
13 changes: 9 additions & 4 deletions ...rest-high-level/src/test/java/org/elasticsearch/client/ESRestHighLevelClientTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.action.support.PlainActionFuture;
import org.elasticsearch.client.indices.CreateIndexRequest;
import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
Expand Down Expand Up @@ -78,20 +79,22 @@ protected static <Req, Resp> Resp execute(Req request, SyncMethod<Req, Resp> syn
AsyncMethod<Req, Resp> asyncMethod) throws IOException {
return execute(request, syncMethod, asyncMethod, RequestOptions.DEFAULT);
}

/**
* Executes the provided request using either the sync method or its async variant, both provided as functions
*/
protected static <Req, Resp> Resp execute(Req request, SyncMethod<Req, Resp> syncMethod,
AsyncMethod<Req, Resp> asyncMethod, RequestOptions options) throws IOException {
if (randomBoolean()) {
final boolean async = Booleans.parseBoolean(System.getProperty("tests.rest.async", "false"));
Copy link
Member

Choose a reason for hiding this comment

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

I think we can read this system property as a constant?
We either runs all tests with this set to false or true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea def.


if (async == false) {
return syncMethod.execute(request, options);
} else {
PlainActionFuture<Resp> future = PlainActionFuture.newFuture();
asyncMethod.execute(request, options, future);
return future.actionGet();
}
}
}

/**
* Executes the provided request using either the sync method or its async
Expand All @@ -100,7 +103,9 @@ protected static <Req, Resp> Resp execute(Req request, SyncMethod<Req, Resp> syn
*/
protected static <Resp> Resp execute(SyncMethodNoRequest<Resp> syncMethodNoRequest, AsyncMethodNoRequest<Resp> asyncMethodNoRequest,
RequestOptions requestOptions) throws IOException {
if (randomBoolean()) {
final boolean async = Booleans.parseBoolean(System.getProperty("tests.rest.async", "false"));

if (async == false) {
return syncMethodNoRequest.execute(requestOptions);
} else {
PlainActionFuture<Resp> future = PlainActionFuture.newFuture();
Expand Down
8 changes: 7 additions & 1 deletion ...l/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import org.elasticsearch.client.indices.CreateIndexRequest;
import org.elasticsearch.cluster.SnapshotsInProgress;
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.xcontent.XContentType;
Expand Down Expand Up @@ -457,7 +458,12 @@ public void testSnapshotVerifyRepository() throws IOException {
List<VerifyRepositoryResponse.NodeView> repositoryMetaDataResponse = response.getNodes();
// end::verify-repository-response
assertThat(1, equalTo(repositoryMetaDataResponse.size()));
assertThat("integTest-0", equalTo(repositoryMetaDataResponse.get(0).getName()));
final boolean async = Booleans.parseBoolean(System.getProperty("tests.rest.async", "false"));
if (async) {
assertThat("asyncIntegTest-0", equalTo(repositoryMetaDataResponse.get(0).getName()));
} else {
assertThat("integTest-0", equalTo(repositoryMetaDataResponse.get(0).getName()));
}
}

public void testSnapshotVerifyRepositoryAsync() throws InterruptedException {
Expand Down