Expand retry options

[apurvis]

@slpsys i am noticing that pester doesn't exactly match our use case for all the stuff currently retried in phorklift / DW - the main difference is that a lot of the retries want a match both on the class of the error AND on the message fragment

[apurvis]

@dollschasingmen

[apurvis]

i went with determination of retry instead of determination of reraise... seems a little more straightforward?

[apurvis]

oh also importantly, now you can pass an array of retry_error_messages instead of just a single string/regex.

[dollschasingmen]

i find this confusing -- maybe this more straightfoward?

  class_retry = retry_error_classes && retry_error_classes.include?(e.class)
  message_retry = retry_error_messages && !retry_error_messages.any? { |m| e.message[m] }
  retry = message_retry && class_retry

[apurvis]

that doesn't quite cut it unfortunately but yeah i agree it feels too
confusing.

if there ARE NOT provided retry_error_classes, a retry_error_messages entry
would be sufficient to cause a retry on its own.

if there ARE provided retry_error_classes AND retry_error_messages, both
the class and the message should match to retry (this is how a lot of our
retries are in the DW/now phorklift)

On Wed, Apr 1, 2015 at 12:20 PM, dollschasingmen [email protected]
wrote:

In lib/pester.rb
#5 (comment):

@@ -56,12 +55,20 @@ def self.retry_action(opts = {}, &block)
result = yield block
return result
rescue => e

•    class_reraise = opts[:retry_error_classes] && !opts[:retry_error_classes].include?(e.class)
  

•    retry_error_classes = opts[:retry_error_classes]
  

•    retry_error_messages = opts[:retry_error_messages]
  

•    class_retry = retry_error_classes && retry_error_classes.include?(e.class)
  

•    if class_retry && retry_error_messages && !retry_error_messages.any? { |m| e.message[m] }
  

•      class_retry = false
  

•    end
  

i find this confusing -- maybe this more straightfoward?
class_retry = retry_error_classes && retry_error_classes.include?(e.class)
message_retry = retry_error_messages && !retry_error_messages.any? { |m|
e.message[m] }
retry = message_retry && class_retry

—
Reply to this email directly or view it on GitHub
https://github.com/lumoslabs/pester/pull/5/files#r27602011.

[dollschasingmen]

jaisus

[dollschasingmen]

if retry_error_classes && !retry_error_messages
  retry =  retry_error_classes.include?(e.class)
elsif retry_error_messages && !retry_error_classes
  retry = retry_error_messages.any? { |m| e.message[m] }
elsif retry_error_messages && retry_error_classes
  retry = retry_error_messages.any? { |m| e.message[m] } && retry_error_classes.include?(e.class)
else
  retry = false
end

[apurvis]

yeah that's clearer; i'll change it.

[slpsys]

Can we just bust the decision-making code into a different method with its own specs? This would get around the ABC rubocop warning I've been ignoring, because I haven't had a chance to actually, substantively refactor the code to just return a boolean.

[apurvis]

sure

On Wed, Apr 1, 2015 at 1:17 PM, Marc Bollinger [email protected]
wrote:

In lib/pester.rb
#5 (comment):

•    retry_error_messages = opts[:retry_error_messages]
  

•    reraise_error_classes = opts[:reraise_error_classes]
  

•    if retry_error_classes
  

•      if retry_error_messages
  

•        retry_decision = retry_error_classes.include?(e.class) && retry_error_messages.any? { |m| e.message[m] }
  

•      else
  

•        retry_decision = retry_error_classes.include?(e.class)
  

•      end
  

•    elsif retry_error_messages
  

•      retry_decision = retry_error_messages.any? { |m| e.message[m] }
  

•    elsif reraise_error_classes && reraise_error_classes.include?(e.class)
  

•      retry_decision = false
  

•    else
  

•      retry_decision = true
  

•    end
  

Can we just bust the decision-making code into a different method with its
own specs? This would get around the ABC rubocop warning I've been
ignoring, because I haven't had a chance to actually, substantively
refactor the code to just return a boolean.

—
Reply to this email directly or view it on GitHub
https://github.com/lumoslabs/pester/pull/5/files#r27607050.

[apurvis]

ok i'll go with unless

[apurvis]

k i put the proc stuff on a separate PR #6
this one look good to you guys?

[slpsys]

yeah, this reads much more nicely. replace those "s with '? that warning is still on in rubocop

[apurvis]

oops sorry hang on

On Wed, Apr 1, 2015 at 2:48 PM, Marc Bollinger [email protected]
wrote:

In lib/pester.rb
#5 (comment):

   rescue => e

•    class_reraise = opts[:retry_error_classes] && !opts[:retry_error_classes].include?(e.class)
  

•    reraise_error = opts[:reraise_error_classes] && opts[:reraise_error_classes].include?(e.class)
  

- message_reraise = opts[:message] && !e.message[opts[:message]]

•    if class_reraise || message_reraise || reraise_error
  

•      match_type = class_reraise ? 'class' : 'message'
  

•      opts[:logger].warn("Reraising exception from inside retry_action because provided #{match_type} was not matched.")
  

•    unless should_retry?(e, opts)
  

•      opts[:logger].warn("Reraising exception from inside retry_action.")
     raise
   end
  

yeah, this reads much more nicely. replace those "s with '? that warning
is still on in rubocop

—
Reply to this email directly or view it on GitHub
https://github.com/lumoslabs/pester/pull/5/files#r27615408.