On 21 May 2014 08:27, GESCONSULTOR - Óscar Bou <[email protected]>wrote:
> Hi, Dan. > > Mmmm... not sure if the transaction should abort every time there's an > exception on an event handler. That was the main argument on the Guava's > thread. > I'll comment in-line; basically I agree with you that we could be more sophisticated, however some of the options below are non-trivial. I'd rather get there incrementally, ie commit what I have implemented to date. And (so I'm clear) what I have implemented thus far is that if the subscriber throws an exception, the xactn is aborted. > As the events are something gone in the future, AFTER the main "thing" > that have caused the Event to be posted has happen, there can be 3 options: > 1. We want that they don't affect the previous code that dispatched the > event (so transaction shouldn't be aborted - in fact it could be commited > before dispatching. This seems to be the main argument on Guava's thread). > So, this is the main use case that the implementation does not support (and the main change in behaviour). The work-around - as you point out below, in fact - is to wrap any existing code in a try...catch. > 2. We consider the event handlers work as part of the transaction, as a > way to decouple code (that's my case when building this projection; as it's > in-process I prefer it to be transactionally consistent than eventually > consistent. I think that most users will expect this when using Isis > EventBus in-process. As I see, you've implemented this. And the Axon's > SimpleBus in-process implementation, also is implemented this way [1]). > Yes, this is what is implemented. > 3. Send an "special event" to a "special event handler" and let the user > decide how to handle that exception (seems this was the final consensus on > the Guava's thread). > > That's more of an implementation detail for how to obtain either of the above behaviours, I think. > Reading the thread, I think that in order to admit both options, finally > they arrived to the solution of the SubscriberExceptionHandler, to make it > pluggable. > > With your proposal, in order to have the first option, we can always > surround the event handler's code inside a "big" try..catch(Exception e) , > ensuring no Exception is thrown that can rollback the transaction. > > Exactly. I appreciate that adds some boilerplate to all subscribers. > Or better, I see there's a SubscriberExceptionContext. > > We could evaluate the method that raised the exception looking for an > annotation like @IndependentTransaction, @NewTransaction or similar, and, > in that case, not abort it. If not, the behavior you implemented. > > What do you think? > > It's a reasonable idea, but it'll still add a boilerplate annotation to every subscriber. Also, actually creating a separate Isis transaction for the event handling "phase" (which is what those annotations names imply) would be quite tricky to handle as Isis is currently designed. Realistically it would require Isis to be implemented on top of JEE7, which is something for the Isis 2.x or even Isis 3.x timeframe. We could add an @IgnoreAnyExceptions annotation, which would be more accurate, but it still feels like rather obscure compared to a simple try... catch in the method body. But if we do want to add such an annotation, it's an easy enough enhancement, for which I suggest a separate ticket to implement. > Simply to notice, this week there's an interview with the author of Axon > on InfoQ [2]. > > Thx, worth reading. Cheers Dan > Regards, > > Oscar > > > > [1] http://www.axonframework.org/docs/2.1/single.html#event-processing > [2] http://www.infoq.com/interviews/allard-buijze-cqrs-event-sourcing > > > > > El 21/05/2014, a las 08:50, Dan Haywood <[email protected]> > escribió: > > > > On 18 May 2014 17:11, GESCONSULTOR - Óscar Bou <[email protected]>wrote: > >> >> I've been working changing the current implementation to a new one based >> on the EvenBus the whole weekend and it works wonderfully :-)) >> >> > very coo. > > > >> With current implementation I have had enough for all my use cases. >> >> There's just one observation related to Exception handling on event >> subscribers. >> >> By default, Guava's EventBus will hidden any exception thrown by an event >> subscriber. In last implementations (version 16) they allowed a method to >> create the EventBus with a SubscriberExceptionHandler [1]. >> >> There's a discussion about its implementation on [2]. >> >> > Hi Oscar, > > My thoughts on this is that the subscribing service should be able to veto > the interaction by throwing different sorts of exception: > - a RecoverableException (or ApplicationException, its subclass) should > abort the transaction and render a user-friendly error > - a NonRecoverableException (or any other RuntimeException, really) should > abort the transaction and be treated as an unexpected/serious error. > > Just playing around with this now... the code is something like: > > protected EventBus newEventBus() { > return new EventBus(newEventBusSubscriberExceptionHandler()); > } > > protected SubscriberExceptionHandler > newEventBusSubscriberExceptionHandler() { > return new SubscriberExceptionHandler(){ > @Override > public void handleException(Throwable exception, > SubscriberExceptionContext context) { > getTransactionManager().getTransaction().setAbortCause(new > IsisApplicationException(exception)); > } > }; > } > > I then use the ExceptionRecognizer infrastructure to do the render either > as non-fatal or fatal error. > > Need to test how this plays with the integration tests, but is working ok > with the Wicket viewer. > > Thoughts? > > Dan > > > > > >> If not present, in order to, at least, show your exception on the log, >> you must implement something like: >> >> @Programmatic >> @Subscribe >> public void on(final AssetCollectionWithRelationshipAddedToEvent >> event) { >> LOG.debug("Event Received: {}", event.toString()); >> >> // Event Handlers should not throw any exceptions. See: >> // >> http://stackoverflow.com/questions/9233966/guava-eventbus-dont-catch-runtimeexception >> try { >> this.upsertRelationshipFromEvent(event.getSource(), >> event.getValue(), event.getIdentifier().getMemberName()); >> } catch (final Exception e) { >> e.printStackTrace(); >> } >> >> } >> >> >> Not sure about what would be the best implementation. I don't see sample >> implementations neither... >> >> Regards, >> >> Oscar >> >> >> >> [1] >> https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/common/eventbus/EventBus.java#190 >> [2] https://code.google.com/p/guava-libraries/issues/detail?id=766 >> >> >> >> >> El 17/05/2014, a las 09:57, Dan Haywood <[email protected]> >> escribió: >> >> >> On 17 May 2014 07:42, GESCONSULTOR <[email protected]> wrote: >> >> Hi Dan. >> >> Yes, I'm receiving old messages also... >> >> >> thx for the confirmation on that. Wil have to monitor things, see if >> improves. >> >> >> >> That's related to DN, not to the EventBus. >> >> >> OK, glad the EventBus thing is working fine. Think it's a really >> important >> feature that you and I have implemented there, hoping to use it within >> Estatio before too long. >> >> >> >> >> The problem is not with the semantics of the operation, but with it >> "eating" a database exception and returning simply false instead of >> throwing it to be catched anywhere. >> >> The point is that at finish the user cannot be sure that the operation is >> retiring false only because the set contained the object. >> >> There's another case when it returns false: a database exception was >> thrown so the object has not been persisted to the database. >> >> >> I'm still not convinced that DN is wrong here. Yes, an exception gets >> thrown and swallowed/converted to false, but I see that as an >> implementation detail; DN detects the uniqueness constraint and from that >> correctly concludes that the object is already present and therefore >> should >> return false. >> >> Perhaps we should look at this the other way around... what should the >> user >> see in different scenarios: >> >> Here's what I think is the current behaviour: >> >> given the reference is not in the collection >> when the reference is added >> then add() returns true >> and then user sees ...??? >> >> given the reference is already in the collection >> when the reference is added >> then add() returns false >> and then user sees ...??? >> >> given (the reference may or may not be in the collection) >> when an attempt is made to add the reference >> and when a system exception occurs (eg database server down) >> then add() should throw an exception. >> and then user sees ...??? >> >> Dan >> >> >> >> >> HTH, >> >> Oscat >> >> >> >> >> Disculpa que sea breve. >> Enviado desde mi móvil >> >> El 16/05/2014, a las 16:39, Dan Haywood <[email protected]> >> >> escribió: >> >> >> On 16 May 2014 09:43, GESCONSULTOR - Óscar Bou <[email protected] >> wrote: >> >> >> Hi all. >> >> Hi Oscar, >> this has only just come through to my mailbox, even though you sent it ~5 >> hours ago. I've also been having a variety of commit messages etc >> >> delayed, >> >> some by as much as 5 days(!). Not sure if the problem is at my end or at >> ASFs (but I suspect perhaps the latter...) >> >> Have you noticed any delays? >> >> >> >> I'm implementing the new support for automatic simple and collection >> properties change events (@PostsPropertyChangedEvent, @ >> PostsCollectionAddedToEvent, @PostsCollectionRemovedFromEvent) and the >> >> new >> >> mechanism works really nice :-)) >> >> I've just initially forgot to register the service as an event >> >> subscriber, >> >> thinking that it was unnecessary. So perhaps auto-registering the >> >> service >> >> when "detecting" the guava's @Subscribe annotation would be a good >> enhancement. >> >> OK, I'll raise a ticket. >> >> >> >> >> I've found that an exception thrown by DN has been hidden by Isis. >> >> Just so I'm clear ... this is unrelated to the event bus stuff, correct? >> >> >> >> >> The root-cause is because I have not properly defined the @Inheritance >> mappings correctly, but the relevant thing from Isis perspective is >> >> that DN >> >> does not throw any exception if, on a collection annotated with @Join, >> >> the >> >> "add" is not properly executed. >> >> >> When the >> >> this.getAggregatedServices().add(service); >> >> >> Is called, the following exception occurs: >> [snip] >> But DN does not throw any exception. It simply returns false when >> >> exiting >> >> the >> >> this.getAggregatedServices().add(service); >> >> method ... >> >> The DN implementation is on [1]. >> >> So seems that it's mandatory to evaluate the result of "add" !!!! >> >> I don't think this is a bug, I believe that DN is conforming to the >> semantics of Set#add(...). I've just mailed Andy @ DN for clarification. >> >> >> >> I understand that an ApplicationException should be raised in all cases. >> What would be a convenient "idiom" we can all use? >> >> From the Javadocs: >> >> Adds the specified element to this set if it is not already present >> (optional operation). More formally, adds the specified element e to this >> set if the set contains no element e2 such that (e==null ? e2==null : >> e.equals(e2)). If this set already contains the element, the call leaves >> the set unchanged and returns false. >> >> If it returns false, it means that the element is already in the set; the >> post-condition is the same. So the idiom is simply to ignore the return >> code, it doesn't matter if true of false is returned. >> >> >> Of course, this relies on a correct implementation of equals(); one >> >> option >> >> is to use Isis' ObjectContracts helper class. >> >> >> HTH >> Dan >> >> >> >> >> >> Thanks, >> >> Oscar >> >> >> >> >> >> >> [1] >> >> >> https://github.com/datanucleus/datanucleus-core/blob/master/src/main/java/org/datanucleus/store/types/wrappers/backed/TreeSet.java#L719 >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> Óscar Bou Bou >> Responsable de Producto >> Auditor Jefe de Certificación ISO 27001 en BSI >> CISA, CRISC, APMG ISO 20000, ITIL-F >> >> <contactenos.html.gif> 902 900 231 / 620 267 520 >> <Pasted Graphic 1.tiff> http://www.twitter.com/oscarbou >> >> <gesdatos-software.gif> http://es.linkedin.com/in/oscarbou >> >> <blog.png> http://www.GesConsultor.com <http://www.gesconsultor.com/> >> >> <gesconsultor_logo_blue_email.png> >> >> >> Este mensaje y los ficheros anexos son confidenciales. Los mismos >> contienen información reservada que no puede ser difundida. Si usted ha >> recibido este correo por error, tenga la amabilidad de eliminarlo de su >> sistema y avisar al remitente mediante reenvío a su dirección electrónica; >> no deberá copiar el mensaje ni divulgar su contenido a ninguna persona. >> Su dirección de correo electrónico junto a sus datos personales constan >> en un fichero titularidad de Gesdatos Software, S.L. cuya finalidad es la >> de mantener el contacto con Ud. Si quiere saber de qué información >> disponemos de Ud., modificarla, y en su caso, cancelarla, puede hacerlo >> enviando un escrito al efecto, acompañado de una fotocopia de su D.N.I. a >> la siguiente dirección: Gesdatos Software, S.L. , Paseo de la Castellana, >> 153 bajo - 28046 (Madrid), y Avda. Cortes Valencianas num. 50, 1ºC - 46015 >> (Valencia). Asimismo, es su responsabilidad comprobar que este mensaje o >> sus archivos adjuntos no contengan virus informáticos, y en caso que los >> tuvieran eliminarlos. >> >> >> >> >> >> > > > *Óscar Bou Bou* > Responsable de Producto > Auditor Jefe de Certificación ISO 27001 en BSI > CISA, CRISC, APMG ISO 20000, ITIL-F > > 902 900 231 / 620 267 520 > http://www.twitter.com/oscarbou > > http://es.linkedin.com/in/oscarbou > > http://www.GesConsultor.com <http://www.gesconsultor.com/> > > > > Este mensaje y los ficheros anexos son confidenciales. Los mismos > contienen información reservada que no puede ser difundida. Si usted ha > recibido este correo por error, tenga la amabilidad de eliminarlo de su > sistema y avisar al remitente mediante reenvío a su dirección electrónica; > no deberá copiar el mensaje ni divulgar su contenido a ninguna persona. > Su dirección de correo electrónico junto a sus datos personales constan en > un fichero titularidad de Gesdatos Software, S.L. cuya finalidad es la de > mantener el contacto con Ud. Si quiere saber de qué información disponemos > de Ud., modificarla, y en su caso, cancelarla, puede hacerlo enviando un > escrito al efecto, acompañado de una fotocopia de su D.N.I. a la siguiente > dirección: Gesdatos Software, S.L. , Paseo de la Castellana, 153 bajo - > 28046 (Madrid), y Avda. Cortes Valencianas num. 50, 1ºC - 46015 (Valencia). > Asimismo, es su responsabilidad comprobar que este mensaje o sus archivos > adjuntos no contengan virus informáticos, y en caso que los tuvieran > eliminarlos. > > > > > >
