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

Retry event handler and process manager on error #20

Closed
slashdotdash opened this issue Oct 18, 2016 · 8 comments
Closed

Retry event handler and process manager on error #20

slashdotdash opened this issue Oct 18, 2016 · 8 comments

Comments

@slashdotdash
Copy link
Member

slashdotdash commented Oct 18, 2016

Event handlers and process managers are expected to return :ok on successful handling of an event. They return {:error, reason} on failure. In this case the event handler should be retried.

Potential error handling options: retry, skip, ignore, stop, or park event.

@Papipo
Copy link

Papipo commented Oct 19, 2016

Maybe skipping the event is not always a viable option depending on the domain. I guess that failing is a must in many scenarios. Maybe there is a bug that has to be fixed and skipping some events leads to inconsistent state.

@slashdotdash
Copy link
Member Author

@Papipo Skip event on error could be made configurable, either per event handler or globally.

@slashdotdash
Copy link
Member Author

Could use tarearbol to provide retry logic.

@drozzy
Copy link

drozzy commented Aug 22, 2017

I see no need for this.

  1. Crash the app.
  2. App restarts as defined pre restart policy.
  3. Event gets delivered again.
  4. App may crash again. After erlang-defined crash semantics exceed the threshold the BEAM shuts down.
  5. Upper level machinery (e.g. docker) may restart the server.

This works great for my own cases (where I hand-built my process managers). Can't see why commanded would not succeed with this simple approach as well.

@astery
Copy link
Contributor

astery commented Aug 23, 2017

@drozzy, I think you talking about scenario when process manager generally fails (all process manager instances are failed), it's definitely should result app crash.

And I think proposal about - why crash the whole application if only one specific process manager instance fails? Just submit log, and stop executing bad process manager instance, or try another restart policy (like dropping state or skip event).

@slashdotdash
Copy link
Member Author

slashdotdash commented Aug 23, 2017

@drozzy Providing an extension point allows consumers of Commanded finer control over how they deal with errors on a case by case basis. So you could decide that for a certain event handler, or particular event, it should retry X times with exponential back-off between retries. Another handler could simply skip any problematic events. You could do this yourself inside an event handler, I'm proposing that Commanded provides a convenient interface for you to use.

I don't know exactly what the best strategy is for each use case, therefore make it user configurable.

@drozzy
Copy link

drozzy commented Aug 24, 2017

Seems like this can be done with some external dependency in client code. Is there any actual use cases for this?

P.S.: I quite like Gregg's approach to features.

@slashdotdash slashdotdash changed the title Retry event handler and process router on error Retry event handler and process manager on error Oct 12, 2017
@slashdotdash
Copy link
Member Author

Closing this issue. Replaced by #93 to support error handling for process manager command dispatch only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants