Skip to content

Fix sporadic test failure in Elasticsearch53SpringRepositoryTest #1115

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

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

trask
Copy link
Contributor

@trask trask commented Nov 27, 2019

Hi all!

This is helping to resolve the sporadic test failures in Elasticsearch53SpringRepositoryTest, see open-telemetry/opentelemetry-java-instrumentation#28.

@trask trask requested a review from a team as a code owner November 27, 2019 18:47
@trask
Copy link
Contributor Author

trask commented Nov 27, 2019

🙁 starting to see sporadic failures with this change at:

java.util.concurrent.TimeoutException: Timeout waiting for 2 trace(s). ListWriter.size() == 1
	at datadog.trace.common.writer.ListWriter.waitForTraces(ListWriter.java:43)
	at springdata.Elasticsearch53SpringRepositoryTest.waitForTracesAndSortSpans(Elasticsearch53SpringRepositoryTest.groovy:430)
	at springdata.Elasticsearch53SpringRepositoryTest.test CRUD(Elasticsearch53SpringRepositoryTest.groovy:115)

I will investigate further and post back here.

@randomanderson
Copy link
Contributor

This one seems similar to the sporadic Couchbase test failures that took multiple attempts (#998, #992, #995, #996, #1010, #1021) to get to work.

One change that helped was splitting test CRUD into -> "test save and retrieve", "test save and update", and "test save and delete". (43cbf7a)

One of the other keys found later was that repo.deleteAll() creates a variable number of traces depending on the state of the repo so having it in setup() was impossible. Each test needs to call deleteAll itself and wait for those traces (see most recent version CouchbaseSpringRepositoryTest)

Copy link
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

@randomanderson I propose we merge this PR for now and continue working with your suggested fix in a future PR. Objections?

@randomanderson
Copy link
Contributor

@tylerbenson I agree. Sorting the spans is an improvement.

@randomanderson randomanderson merged commit 7717dbf into DataDog:master Dec 10, 2019
@tylerbenson tylerbenson added this to the 0.39.0 milestone Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants