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

Avoid listener blocking #1482

Merged

Conversation

waterdrink
Copy link
Contributor

close #1469

@stuartnelson3 stuartnelson3 self-requested a review July 27, 2018 13:04
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

@waterdrink do I understand this change correctly, that instead of sending all alerts to all iterators, including those that are already done, it only sends alerts to not-yet-done listeners and skips the others via case <-l.done:? I guess a listener can not be removed during that time, as Puts() holds the lock?

Not related, you are missing the sign-off line in your commit message, see Details on the DCO failure. Simply do a git commit --amend --no-edit -s and push the new commit.

@waterdrink
Copy link
Contributor Author

@mxinden The listener do can not be removed when Puts() holds the lock, but listener can not receive alerts from Puts() when Subscribe is done but listener has not removed yet. So it will block at https://github.com/prometheus/alertmanager/blob/master/provider/mem/mem.go#L100

@waterdrink
Copy link
Contributor Author

My fault. I messed up the pull request commit, and have no idea why ci is failed at config_test.go.I can close this one and start a new pull request if needed.

@mxinden
Copy link
Member

mxinden commented Jul 30, 2018

and have no idea why ci is failed at config_test.go.

This is not related to your patch and will be fixed with #1486.

@stuartnelson3
Copy link
Contributor

My fault. I messed up the pull request commit, and have no idea why ci is failed at config_test.go.I can close this one and start a new pull request if needed.

It's fine to keep working in this PR.

@waterdrink waterdrink closed this Aug 5, 2018
@waterdrink waterdrink reopened this Aug 5, 2018
Signed-off-by: wangyue <wangyue@actiontech.com>
@waterdrink waterdrink force-pushed the avoid-listener-blocking branch from 66b76fa to 6334c31 Compare August 5, 2018 08:57
@waterdrink
Copy link
Contributor Author

hi, I did rebase for ease of viewing.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

This looks good to me. #1503 adds a test failing without this fix.

Still leaving @stuartnelson3 and @simonpasquier some time to have a look before merging.

Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

sorry for the delay, thanks for adding this

@stuartnelson3 stuartnelson3 merged commit 0fc0ff8 into prometheus:master Aug 6, 2018
@mxinden
Copy link
Member

mxinden commented Aug 6, 2018

@waterdrink great catch! Thanks for the contribution.

mxinden pushed a commit to mxinden/alertmanager that referenced this pull request Aug 14, 2018
Signed-off-by: wangyue <wangyue@actiontech.com>
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.

blocking when alert
3 participants