Skip to content

Add sleep to allow ES sufficient time for CCR#11172

Merged
ruflin merged 2 commits intoelastic:masterfrom
ycombinator:mb-es-ccr-flakiness
Mar 13, 2019
Merged

Add sleep to allow ES sufficient time for CCR#11172
ruflin merged 2 commits intoelastic:masterfrom
ycombinator:mb-es-ccr-flakiness

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

@ycombinator ycombinator commented Mar 9, 2019

After repeatedly running the Elasticsearch module integration test in Metricbeat, I found that sometimes Elasticsearch doesn't get enough time to perform CCR and generate CCR stats. This causes the following error, but only some times:

--- FAIL: TestFetch (2.44s)
    --- FAIL: TestFetch/ccr (0.08s)
        elasticsearch_integration_test.go:92:
                Error Trace:    elasticsearch_integration_test.go:92
                Error:          Should NOT be empty, but was []
                Test:           TestFetch/ccr

So this PR adds a 300ms sleep to give Elasticsearch enough time to perform CCR and generate CCR stats. After testing various sleep durations, I found that 300ms seemed to be the lowest (round) value I could use that consistently passed this test.

Possibly related: #10866

@ycombinator ycombinator requested a review from a team as a code owner March 9, 2019 00:59
@ycombinator ycombinator added module review Metricbeat Metricbeat :Testing flaky-test Unstable or unreliable test cases. labels Mar 9, 2019
@ycombinator ycombinator requested a review from sayden March 9, 2019 00:59
Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM, especially as the build is green.

In general not a big fan of time based timeouts as in the past this often has lead to flaky tests on it's own. Instead, could we instead have a "wait for" approach that checks the api every 100ms for a max of 5s and then returns an error?

I'm good with merging as is as it already adresses an issue but would be good to have long term a nicer solution.

@ycombinator
Copy link
Copy Markdown
Contributor Author

@ruflin Agreed that waitFor is better than sleep. Will implement in this PR.

@ycombinator
Copy link
Copy Markdown
Contributor Author

@ruflin Implemented waitFor pattern in 8d8aabf. Ready for review again. Thanks!

@ruflin ruflin merged commit 672cf1b into elastic:master Mar 13, 2019
@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Mar 13, 2019

@ycombinator Already merged so today we can see if it has an effect on new PR's.

@ycombinator
Copy link
Copy Markdown
Contributor Author

Thanks @ruflin!

ycombinator added a commit that referenced this pull request Jun 6, 2019
* Add sleep to allow ES sufficient time for CCR (#11172)

After repeatedly running the Elasticsearch module integration test in Metricbeat, I found that sometimes Elasticsearch doesn't get enough time to perform CCR and generate CCR stats. This causes the following error, but only some times:

```
--- FAIL: TestFetch (2.44s)
    --- FAIL: TestFetch/ccr (0.08s)
        elasticsearch_integration_test.go:92:
                Error Trace:    elasticsearch_integration_test.go:92
                Error:          Should NOT be empty, but was []
                Test:           TestFetch/ccr
```

So this PR adds a 300ms sleep to give Elasticsearch enough time to perform CCR and generate CCR stats. After testing various sleep durations, I found that 300ms seemed to be the lowest (round) value I could use that consistently passed this test.

Possibly related: #10866

* Fixing formatting
@ycombinator ycombinator deleted the mb-es-ccr-flakiness branch December 25, 2019 11:17
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…lastic#12437)

* Add sleep to allow ES sufficient time for CCR (elastic#11172)

After repeatedly running the Elasticsearch module integration test in Metricbeat, I found that sometimes Elasticsearch doesn't get enough time to perform CCR and generate CCR stats. This causes the following error, but only some times:

```
--- FAIL: TestFetch (2.44s)
    --- FAIL: TestFetch/ccr (0.08s)
        elasticsearch_integration_test.go:92:
                Error Trace:    elasticsearch_integration_test.go:92
                Error:          Should NOT be empty, but was []
                Test:           TestFetch/ccr
```

So this PR adds a 300ms sleep to give Elasticsearch enough time to perform CCR and generate CCR stats. After testing various sleep durations, I found that 300ms seemed to be the lowest (round) value I could use that consistently passed this test.

Possibly related: elastic#10866

* Fixing formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky-test Unstable or unreliable test cases. Metricbeat Metricbeat module review :Testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants