Skip to content
This repository has been archived by the owner on Nov 20, 2019. It is now read-only.

handler must fire resolve if it fired for create #106

Conversation

cabecada
Copy link
Contributor

#103
handler should now fire a resolve if the event was triggered
for create action earlier.
handler should not fire a resolve if the event was not triggered
for create action earlier.

Yelp#103
handler should now fire a resolve if the event was triggered
for create action earlier.
handler should not fire a resolve if the event was not triggered
for create action earlier.
files/base.rb Outdated
@@ -229,6 +229,18 @@ def filter_repeated

initial_failing_occurrences = interval > 0 ? (alert_after / interval) : 0
number_of_failed_attempts = @event['occurrences'] - initial_failing_occurrences
# sensu 0.26+ only, else never make occurrences_watermark actionable
occurrences_watermark = @event.key?('occurrences_watermark') ? event['occurrences_watermark'] : -1
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put all of your logic into if @event.key?('occurrences_watermark') instead of assigning it a phony value of -1.

files/base.rb Outdated
# occurrences_watermark > initial_failing_occurrences prove enough occurrences
# of the event that triggered create and hence it is safe to filter the event to resolve handler action.
if occurrences_watermark > initial_failing_occurrences and @event['action'] == 'resolve'
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

What's your reasoning behind putting all this code in filter_repeated method? And could you please confirm that returning nil from filter_repeated is somehow significant - in other words, could you please clarify why you are returning nil here.

Copy link
Contributor

@fessyfoo fessyfoo Mar 15, 2017

Choose a reason for hiding this comment

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

I'm guessing because that's the style further down in this method where the exponential back off returns nil to exit the method and indicate "ok to process". I think both locations could probably just use return as that's effectively the same thing. further nuance, while the tests are looking for nil I think any return that doesn't call #bail would allow the event to be processed.

        if power_of_two?(number_of_failed_attempts)
          # Then This is our MOMENT!
          return nil
        else

@cabecada
Copy link
Contributor Author

@solarkennedy @somic style changes made.
Also, are you convinced with the comments from @fessyfoo ?
Can this be merged now?

@solarkennedy
Copy link
Contributor

Lets do it

@solarkennedy solarkennedy merged commit d568297 into Yelp:master Mar 21, 2017
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.

4 participants