Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Use coroutines for AlertMover and MonitorRunner #11

Merged
3 commits merged into from May 15, 2019
Merged

Use coroutines for AlertMover and MonitorRunner #11

3 commits merged into from May 15, 2019

Conversation

ghost
Copy link

@ghost ghost commented Mar 22, 2019

This change converts the MonitorRunner and AlertMover to use kotlin coroutines.

  • MonitorRunner's blocking implementation and large thread pool have been removed without having to switch to a full callback style of API. We no longer allocate & block a thread for the entire duration we run a monitor. We no longer block a thread when performing I/O on the ES cluster (performing the search or reading/writing alerts). The only blocking I/O that remains is when publishing notifications which is done on a much smaller background I/O threadpool.
  • AlertMover's callback based implementation is simplified considerably by coroutines.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ghost ghost requested review from dbbaughe, jinsoor-amzn and lucaswin-amzn March 22, 2019 10:27
@dbbaughe
Copy link
Contributor

@vinooamzn Hi Vinoo, can you resolve the conflicts in this.

@jinsoor-amzn I believe you ran performance tests before for alerting to see the impact on a cluster, do you think it would be possible to run similar performance tests comparing alerting w/ threads and alerting w/ coroutines to confirm there is no negative impact for switching.

dbbaughe
dbbaughe previously approved these changes May 9, 2019
Copy link
Contributor

@dbbaughe dbbaughe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes Vinoo

lucaswin-amzn
lucaswin-amzn previously approved these changes May 9, 2019
Copy link
Contributor

@lucaswin-amzn lucaswin-amzn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making this change! :)

@ghost ghost dismissed stale reviews from lucaswin-amzn and dbbaughe via d8991cb May 14, 2019 04:24
@ghost ghost merged commit 56c49d9 into opendistro-for-elasticsearch:master May 15, 2019
@ghost ghost deleted the coroutines branch May 15, 2019 03:54
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants