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

Fix flakey apache module timeout test #8028

Merged
merged 1 commit into from Aug 21, 2018

Conversation

Projects
None yet
2 participants
@andrewvc
Contributor

andrewvc commented Aug 20, 2018

This attempts to fix a flakey apache module test by making it less sensitive to timing issues.

Before this patch there was only 50ms for slack for the timeout to happen. My thesis here is that under contention > 50ms of delay was introduced, likely by the Kernel thread scheduler.

By switching to a wait group we have a more deterministic test. Additionally, we now cleanup the server go routine more precisely. It now ends exactly when the test is done, instead of us having it hang around for a fixed interval.

Fixes #7726

Fix flakey apache module timeout test
This attempts to fix a flakey apache module test by making it less sensitive to timing issues.

Before this patch there was only 50ms for slack for the timeout to happen. My thesis here is that under contention > 50ms of delay was introduced, likely by the Kernel thread scheduler.

By switching to a wait group we have a more deterministic test. Additionally, we now cleanup the server go routine more precisely. It now ends exactly when the test is done, instead of us having it hang around for a fixed interval.

Fixes #7726
@andrewvc

This comment has been minimized.

Show comment
Hide comment
@andrewvc

andrewvc Aug 20, 2018

Contributor

I should mention I was not able to repro this locally without artificially constraining the current delay, but CI servers are notorious for being under contention, while developer laptops are not.

Contributor

andrewvc commented Aug 20, 2018

I should mention I was not able to repro this locally without artificially constraining the current delay, but CI servers are notorious for being under contention, while developer laptops are not.

@andrewvc andrewvc added the review label Aug 20, 2018

@ruflin ruflin merged commit 3025f89 into elastic:master Aug 21, 2018

6 checks passed

CLA Commit author has signed the CLA
Details
Hound No violations found. Woof!
beats-ci Build finished.
Details
codecov/patch Coverage not affected when comparing ca8f56b...76077f0
Details
codecov/project 64.6% (+<.01%) compared to ca8f56b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Aug 21, 2018

Collaborator

Thanks for the fix. Let's see if it still happens. Probably worth to also backport this to 6.x to make the tests there more stable.

Collaborator

ruflin commented Aug 21, 2018

Thanks for the fix. Let's see if it still happens. Probably worth to also backport this to 6.x to make the tests there more stable.

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