Remove random when using HLRC sync and async calls #48211

hub-cap · 2019-10-17T19:16:41Z

This commit removes the randomization used by every execute call in the
high level rest tests. Previously every execute call, which can be many
calls per single test, would rely on a random boolean to determine if
they should use the sync or async methods provided to the execute
method. This commit runs the tests twice, using two different clusters,
both of them providing the value one time via a sysprop. This ensures
that the whole suite of tests is run using the sync and async code
paths.

Closes #39667

This commit removes the randomization used by every execute call in the high level rest tests. Previously every execute call, which can be many calls per single test, would rely on a random boolean to determine if they should use the sync or async methods provided to the execute method. This commit runs the tests twice, using two different clusters, both of them providing the value one time via a sysprop. This ensures that the whole suite of tests is run using the sync and async code paths. Closes elastic#39667

elasticmachine · 2019-10-17T19:16:43Z

Pinging @elastic/es-core-features (:Core/Features/Java High Level REST Client)

hub-cap · 2019-10-17T20:49:10Z

@elasticmachine run elasticsearch-ci/1

martijnvg

LGTM, I left one nit.

martijnvg · 2019-10-18T05:58:54Z

...nt/rest-high-level/src/test/java/org/elasticsearch/client/ESRestHighLevelClientTestCase.java

    /**
     * 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"));


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

alpar-t · 2019-10-18T09:00:31Z

client/rest-high-level/build.gradle

-  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


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.

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

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.

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.

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.

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.

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.

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.

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.

mark-vieira

A big +1 to removing our reliance on random testing and instead making testing of async/sync explicit. We should do this in more places.

mark-vieira · 2019-10-18T17:59:29Z

client/rest-high-level/build.gradle

+
+check.dependsOn(asyncIntegTest)
+
+['integTest', 'asyncIntegTest'].each { integName ->


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 }.

mark-vieira · 2019-10-18T18:00:31Z

client/rest-high-level/build.gradle

-  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


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.

Also fixed minor review comments

hub-cap · 2019-10-22T20:17:54Z

comments addressed, I will assume there is no one stopping me from merging this w/ mvg's +1

hub-cap · 2019-10-23T16:39:09Z

@elasticmachine run elasticsearch-ci/packaging-sample-matrix

…emove_random

This commit removes the randomization used by every execute call in the high level rest tests. Previously every execute call, which can be many calls per single test, would rely on a random boolean to determine if they should use the sync or async methods provided to the execute method. This commit runs the tests twice, using two different clusters, both of them providing the value one time via a sysprop. This ensures that the whole suite of tests is run using the sync and async code paths. Closes #39667

hub-cap added :Core/Features/Java High Level REST Client >refactoring v8.0.0 v7.6.0 labels Oct 17, 2019

martijnvg approved these changes Oct 18, 2019

View reviewed changes

alpar-t reviewed Oct 18, 2019

View reviewed changes

Move the async envvar read

312c2ce

mark-vieira reviewed Oct 18, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into hlrc_remove_random

709e622

Also fixed minor review comments

mark-vieira approved these changes Oct 22, 2019

View reviewed changes

hub-cap added 2 commits October 23, 2019 11:43

Merge branch 'master' of github.com:elastic/elasticsearch into hlrc_r…

2ac6686

…emove_random

Fix a test failure

ec524bd

hub-cap merged commit 18d87e3 into elastic:master Oct 24, 2019

hub-cap added the backport pending label Oct 24, 2019

hub-cap removed the backport pending label Oct 24, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove random when using HLRC sync and async calls #48211

Remove random when using HLRC sync and async calls #48211

hub-cap commented Oct 17, 2019

elasticmachine commented Oct 17, 2019

hub-cap commented Oct 17, 2019

martijnvg left a comment

martijnvg Oct 18, 2019

hub-cap Oct 18, 2019

alpar-t Oct 18, 2019

hub-cap Oct 18, 2019

alpar-t Oct 18, 2019

rjernst Oct 18, 2019

mark-vieira Oct 18, 2019

alpar-t Oct 21, 2019

mark-vieira Oct 21, 2019

hub-cap Oct 22, 2019

mark-vieira Oct 22, 2019

mark-vieira left a comment

mark-vieira Oct 18, 2019

mark-vieira Oct 18, 2019

hub-cap commented Oct 22, 2019

hub-cap commented Oct 23, 2019


		check.dependsOn(asyncIntegTest)

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

Remove random when using HLRC sync and async calls #48211

Remove random when using HLRC sync and async calls #48211

Conversation

hub-cap commented Oct 17, 2019

elasticmachine commented Oct 17, 2019

hub-cap commented Oct 17, 2019

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Oct 22, 2019

hub-cap commented Oct 23, 2019