Closed jiexinhuang closed 7 years ago
Algorithm/behaviour looks good. I think now's a good time to refactor. How might it look with a class dedicated to this concern?
Sorry, bit late to the party. I think having this behaviour exist in the framework is awesome. It provides the missing balance between retry forever and die on failure, which is a nice fallback for transient errors.
To throw some further context at the PR, are you able to provide some reasoning behind why you felt you needed this for RSS? (maybe an example scenario)
@mjward One scenario I can think of is when Recurly is down, the reactor that calls Recurly API to create and update subscriptions will throw exceptions and we do want to retry in this case, but retry every second could potentially cost a lot of our Rollbar quota.
@orien @stevehodgkiss would you mind having another look at these refactor commits?
@orien @stevehodgkiss Could you guys have another look please 😄
Context
ESP process can retry processing an event in case of errors, this is helpful to handle random errors on unreliable dependencies like third party APIs, but retry every second could flood our error trackers like Rollbar and generate unnecessary cost.
Changes
ConstantRetry
,NoRetry
andExponentialBackoffRetry
EventSourcery
to config ErrorHandlerClass, a client app could use built-in error handlers or use a custom one.stop_on_failure
option for ESPProcess, this option is superseded by ErrorHandlerConsideration
This is a non-breaking change and should not cause issues for client apps when upgrading.