Skip to content
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

Stop clearing all watches in the mock trigger engine. #39724

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

jtibshirani
Copy link
Contributor

Our watcher integration tests use ScheduleTriggerEngineMock to give more
control over when watches are triggered. This mock trigger engine maintains a
reference to all registered watches. We've seen a few instances of tests
failing because ScheduleTriggerEngineMock could no longer find a watch that
was previously added (for example #35503, #37882, and #39306)

The issue seems to be that after the tests starts, there can be a change in
local shard routing, which then causes the watch service to be loaded. This in
turn calls ScheduleTriggerEngineMock#pauseExecution, which clears all watches
that this mock object holds. Subsequent calls to
ScheduleTriggerEngineMock#trigger then fail to find the requested watches.

This PR proposes to stop clearing the watches in
ScheduleTriggerEngineMock#pauseExecution. Usually, pausing execution on a
trigger engine will stop scheduled watches from running. However, the mock
trigger engine does not run watches on a schedule, and instead allows them to
be triggered manually. So arguably, no action should be taken in
pauseExecution, and in particular it doesn't make sense to clear the list of
registered watches.

Addresses #35503 and #39306.

@jtibshirani jtibshirani added >test-failure Triaged test failures from CI :Data Management/Watcher labels Mar 5, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@jtibshirani jtibshirani marked this pull request as ready for review March 6, 2019 18:18
@jakelandis jakelandis requested review from gwbrown and hub-cap March 7, 2019 15:40
Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

Good catch! I ran this for a few hours and did not see any errors crop up because of the change.

@jtibshirani
Copy link
Contributor Author

Thanks @hub-cap for the review and the extra testing!

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.

4 participants