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

bosun: Concurrent checks #1231

Merged
merged 1 commit into from
Aug 20, 2015
Merged

bosun: Concurrent checks #1231

merged 1 commit into from
Aug 20, 2015

Conversation

captncraig
Copy link
Contributor

This modifies the schedule in many ways. Each alert runs in its own goroutine.

@kylebrandt
Copy link
Member

Will fix #1228

<-checkRunning
return d, err
}
//TODO: This makes less sense with every alert running independently.
Copy link
Member

Choose a reason for hiding this comment

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

This can just be yanked as a feature. The reason I created it was so when someone "fixed" an alert, they didn't have to wait for it to return to normal. But because queries cover a time range, this is generally pretty futile. My idea to actually fix this problem is delayed close as described in #808

@captncraig
Copy link
Contributor Author

@gbrayut, I would love a thorough review on this if you don't mind.

if s.notifications == nil {
s.notifications = make(map[*conf.Notification][]*State)
if s.pendingNotifications == nil {
s.pendingNotifications = make(map[*conf.Notification][]*State)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not well versed in making maps or appending to them, but this smells like something that might need a lock or some other means of preventing duplicate assignments to the variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does require locking. Anywhere this function is called the lock should be held. We are really bad at specifying those kinds of conventions though. This should almost certainly not be exported if that is the case, and should probably note that.

@captncraig captncraig changed the title WIP: Concurrent checks bosun: Concurrent checks Aug 20, 2015
captncraig pushed a commit that referenced this pull request Aug 20, 2015
@captncraig captncraig merged commit ee496f2 into master Aug 20, 2015
@captncraig captncraig mentioned this pull request Aug 21, 2015
@gbrayut gbrayut deleted the concurrentChecks branch August 24, 2015 17:48
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.

3 participants