Re: [infinispan-dev] Optional in listener events

That is bad and unfortunate because it breaks continuous query and also makes CEP impossible to implement. As I've said, the previous value is absolutely necessary for these use cases. If the notification cannot provide it then (in theory) we have the alternative to make the CQ or CEP engine store the previous value. But that is absurd because it would duplicate the entire contents of the cache inside the CQ/CEP engine. So no. And besides the already highlighted cases where this is broken we've recently added new ones. Some of the functional map write operations also do not provide the previous value. Actually they do not notify the classic @org.infinispan.notifications. Listener at all, in favor of the new org.infinispan.commons.api.functional.Listeners. We badly need to unify these listener interfaces and make sure we always provide the previous value. On 08/29/2016 02:33 PM, Radim Vansa wrote: > On 08/24/2016 04:32 PM, Dan Berindei wrote: >> On Wed, Aug 24, 2016 at 4:14 PM, Adrian Nistor <anistor(a)redhat.com&gt; wrote: >>> I remember there was some discussion to refactor listeners to drop the >>> concept of pre-events and have the previous value available in the >>> post-event (where applicable). I do not remember what decision was made >>> about this. But if we do it, that would be already a big backwards >>> incompatible change, so modifying some defaults regarding the behavior of >>> the Listener annotation is just mildly disturbing. We'll have to >>> apologize/document this anyway and also provide migration advice. >>> >> That discussion died out because we couldn't decide whether we really >> need to maintain the current listeners or we could switch to >> supporting only JCache and FunctionalMap listener APIs. >> >> FWIW I'm all for modernizing the core listeners, removing the pre >> events, and allowing listeners to receive the previous value in the >> post event. I'd also change the API to be interface-based instead of >> annotation-based. >> >>> On 08/22/2016 09:15 PM, William Burns wrote: >>> >>> I like the idea of having a variable to set on the listener annotation. >>> This way we can know for sure if we need to force previous values for some >>> listeners and not for others. >>> >>> It seems the default should be to force the previous value to be more inline >>> with the current behavior, but I fear no one will use the opposite in this >>> case though. What do you guys think? >> Actually, the current behaviour is *not* to force the previous value. >> If you have the entry in the data container, yes, you'll see it in the >> listener, but if the entry is in a store, you won't. Clustered >> listeners do get the previous value even if it's remote, but not if >> the entry is passivated. > So prev values are not working as doc states (silently returning wrong > values) in case that: > - topology changes (command retry) > - persistence is used > > These are quite non-obvious "if"s for users :-/ I'd call listeners a > non-reliable feature. > >>> On Mon, Aug 22, 2016 at 4:31 AM Adrian Nistor <anistor(a)redhat.com&gt; wrote: >>>> Hi Radim, >>>> >>>> Continuous query is built on top of these listeners. CQ _always_ needs >>>> the previous value and it is very convenient in this case that the >>>> command is forced to load the previous value. I imagine there may be >>>> other use cases where we cannot live without the prev value. >> Unfortunately, if a command is retried in a non-tx cache >> (event.isCommandRetried() == true), the listener may receive the new >> value as the previous value. So CQ needs to support this case, or >> we'll have to finally fix it in core. I'd mention >> versioning/tombstones, but I fear Sanne is going to read this and >> derail the thread ;) >> >>>> I think the listener should be able to state if it needs the prev value >>>> at registration time. Maybe add a new attribute in the Listener >>>> annotation? Similar to how we handled Observation. >>>> >> Actually, with the current API, the only way to get the previous value >> is with the pre event, so we could interpret @Listener(observation = >> POST) as a sign that the listener doesn't need the previous value. > But pre events are also unreliable as they just notify that something > may or may not happen in the future. > >>>> Adrian >>>> >>>> On 08/19/2016 11:34 PM, Radim Vansa wrote: >>>>> Hi, >>>>> >>>>> as I am trying to simplify current entry wrapping and distribution code, >>>>> I often find that listeners can get wrong previous value in the event, >>>>> and it sometimes forces the command to load the value even if it is not >>>>> needed for the command. >>>>> >>>>> I am wondering if we should change the previous value in events to >>>>> Optional - we can usually at least detect that we cannot provide a >>>>> reliable value (e.g. after retry due to topology change, or because the >>>>> command did not bothered to load the previous value from cache loader) >>>>> and return empty Optional. >>>>> >> It would be a breaking change without a lot of benefit, so I would not >> make this change. >> >> If we added a getPreviousValue() method for post events, then yes, it >> could return an Optional. I'm not yet sure it's a good fit for >> isCommandRetried() == true, since in that case we usually have a >> previous value, we're just not sure it's the correct one. > What value has a "previous value" when you can't know it's correct? I > see that as the main problem. > > I wouldn't introduce any complexities (listener flags telling if it > needs or does not need previous value) at this point - the less code the > better, until we fix what we already have. > >> >>>>> WDYT? >>>>> >>>>> Radim >>>>> >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev