What do you think about that? Is there simple work around which would make this feature uncalled-for?
If not, just let me know if I have missed anything in the pull request.
Cheers
Sebastian
Hmm, I see a problem with this solution being less then perfect since it would be a bit confusing for people using async sagas…
The workaround to configure the AnnotatedSagaManager like an ordinary spring bean works relatively well.
However I did notice something peculiar.
The way we handle failures in EventHandlers is usually to throw the exception at the message consumer which nacks the message to rabbit retries the transaction. The whole issue with suppressWarnings is that we would like to do the same for sagas, that is if the a SagaEventHandler fails due to some recoverable timing issues, the message should go back on the queue and be retried. This works fairly bad with the current JpaSagaRepository in conjunction with the saga cache, since if the handler that starts the saga throws an exception the saga will end up in the cache even if it is not persisted, and following evens will invoke the event handlers on the the cached instances.
Any thoughts on this issue?
Cheers
Sebastian
Hi Sebastian,
I think the suppress-exceptions implementation looks nice at first glance. In case of async processing, that setting doesn’t make much sense, but that’s ok. In async-mode, exceptions are always suppressed.
I don’t really have an opinion/solution ready for the scenario you describe. In many cases, multiple events are passed to a saga before it is actually persisted. So the Saga Cache (which is actually more a deduplicator than a cache), is not the only “problem” here. My feeling is that it is in general bad to leave a shared object, and especially a saga, in a bad state when throwing an exception. The only solution I currently see to provide a fool-proof implementation is to make a clone of the Saga instance each time before it is invoked, so that you have a valid state to return to. But I think that’s a relatively expensive operation to perform each time.
Cheers,
Allard
Agreed, it is certainly bad to leave a shared object in a bad state when throwing an exception. But unanticipated exceptions sounds pretty hard to completely guard yourself against? This is one of the reasons to having transactions right? : )
An option to cloning before applying every event is discard the bad state upon exceptions, and I guess one way to do that is to completely scrap and recreate the saga manager? This of course requires that you are in a position to replay all the events that was part of rolled back transaction and that made state changes to the saga that has not been persisted. This in turn is solved by not acking to rabbit until after you’ve committed the saga state to the database, and just getting them redelivered. (I am sensing though, that this is not an option in your current setup?)
The major benefit of that solution is that the cost is moved from the normal case to exception case.
Hi Sebastian,
the problem is that a surrounding transaction can be rolled back for various reasons. It doesn’t even have to be the Saga failing. It could be anything else that is participating in the transaction. So the Saga may be left in a consistent state, but is not consistent with the state around it anymore.
Looks like I need a little redesign to make it possible to remove the need for the “deduplicator”.
Cheers,
Allard