Skip to content

StackOverflowError when calling BulkRequest#add#41672

Merged
hub-cap merged 3 commits intoelastic:masterfrom
gdarmont:pr/fix-41668
May 4, 2019
Merged

StackOverflowError when calling BulkRequest#add#41672
hub-cap merged 3 commits intoelastic:masterfrom
gdarmont:pr/fix-41668

Conversation

@gdarmont
Copy link
Copy Markdown
Contributor

Removing of payload in BulkRequest (commit 6f3578a) had a side effect of making BulkRequest.add(DocWriteRequest<?>...) (with varargs) recursive, thus leading to StackOverflowError.
This PR adds a small change in RequestConvertersTests to show the error and the corresponding fix in BulkRequest.

Hope this helps,
Guillaume.

@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-features

@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented Apr 30, 2019

@elasticmachine add to whitelist

@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented Apr 30, 2019

Thank you for finding this!!

@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented Apr 30, 2019

looks like the tests arent compiling.

* What went wrong:
Execution failed for task ':client:rest-high-level:compileTestJava'.
> Compilation failed; see the compiler error output for details.

You can run the tests with ./gradlew :client:rest-high-level:check

@gdarmont
Copy link
Copy Markdown
Contributor Author

I forgot to run that command after checking the tests in IntelliJ. IntelliJ was configured with Eclipse Compiler, not javac, probably the reason the test was compiling locally but not in Gradle.
Sorry about that, it should be fixed now.

I'll keep an eye on CI results, and make updates if needed.

…(DocWriteRequest<?>...) (varargs version)

Removing of payload in BulkRequest (commit 6f3578a) had a side effect of making `BulkRequest.add(DocWriteRequest<?>...)` (with varargs) recursive, thus leading to StackOverflowError.
This PR adds a small change in RequestConvertersTests to show the error and the corresponding fix in `BulkRequest`.
@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented May 1, 2019

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2

@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented May 1, 2019

If these fail again, can you merge the upstream's master onto your branch? We could have muted some flaky tests since you grabbed the source. Your branch is not old, but its always possible that there is a new flaky test!

@gdarmont
Copy link
Copy Markdown
Contributor Author

gdarmont commented May 1, 2019

I've just merged upstream master and ran ./gradlew :client:rest-high-level:check again to ensure tests are passing on my side.

After searching on Jenkins, I found that the failing test org.elasticsearch.xpack.ml.integration.DeleteExpiredDataIT.testDeleteExpiredData of https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-2/12900/ is also failing, with the same reason, on https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+periodic/1124/, which does not contains my changes.
So I don't know if this is a new flaky test which needs to be fixed in another PR.

Again, I'll check CI results when available.

@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented May 1, 2019

CI is a bit unstable today do to a version bump. It might be best to just have me rerun these tomorrow after I become unblocked on some of my PRs as well (ie, ill be watching it all day heh).

@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented May 2, 2019

ok CI is back to normal, can you merge in the newest from upstream/master? It should fix the CI runs.

@gdarmont
Copy link
Copy Markdown
Contributor Author

gdarmont commented May 2, 2019

Merge with upstream master (46d25e6) done. Fingers crossed ;)

@gdarmont
Copy link
Copy Markdown
Contributor Author

gdarmont commented May 2, 2019

Well, new build, another failing test :(
As explained in Jenkins build log, I ran the following command to reproduce on my side : ./gradlew :x-pack:plugin:ml:qa:native-multi-node-tests:integTestRunner --tests "org.elasticsearch.xpack.ml.integration.AutodetectMemoryLimitIT.testTooManyPartitions" -Dtests.seed=6817C77E85CE1594 -Dtests.security.manager=true -Dtests.locale=kea -Dtests.timezone=Asia/Jakarta -Dcompiler.java=12 -Druntime.java=11.

But the test is OK on my side, so I don't know what to do next. (except maybe going towards the famous definition "Insanity is doing the same thing over and over and expecting different results" ;) ).

@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented May 4, 2019

oh, yea your stuff is certainly not causing ML to fail, thats likely a flaky test. We try to squash them as soon as possible to limit the insanity feeling :)

@elasticmachine run elasticsearch-ci/2

@hub-cap
Copy link
Copy Markdown
Contributor

hub-cap commented May 4, 2019

victory, Ill be merging this and backporting it early next week. Great work and thank you for the catch and contribution!

edit: and by merging and backporting i mean backporting as ive just merged it :P

@hub-cap hub-cap changed the title Fixes #41668 : StackOverflowError when calling BulkRequest.add() (varargs version) StackOverflowError when calling BulkRequest#add May 4, 2019
@hub-cap hub-cap merged commit dc999e4 into elastic:master May 4, 2019
@gdarmont
Copy link
Copy Markdown
Contributor Author

gdarmont commented May 4, 2019

Great :) Glad being helpfull.

@gdarmont gdarmont deleted the pr/fix-41668 branch May 4, 2019 15:24
hub-cap pushed a commit to hub-cap/elasticsearch that referenced this pull request May 22, 2019
Removing of payload in BulkRequest (elastic#39843) had a side effect of making
`BulkRequest.add(DocWriteRequest<?>...)` (with varargs) recursive, thus
leading to StackOverflowError. This PR adds a small change in
RequestConvertersTests to show the error and the corresponding fix in
`BulkRequest`.

Fixes elastic#41668
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
Removing of payload in BulkRequest (elastic#39843) had a side effect of making
`BulkRequest.add(DocWriteRequest<?>...)` (with varargs) recursive, thus
leading to StackOverflowError. This PR adds a small change in
RequestConvertersTests to show the error and the corresponding fix in
`BulkRequest`.

Fixes elastic#41668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants