[infinispan-dev] Optional in listener events

[Dan Berindei]

On Wed, Aug 24, 2016 at 4:14 PM, Adrian Nistor <anistor at redhat.com> 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.

>
> On Mon, Aug 22, 2016 at 4:31 AM Adrian Nistor <anistor at redhat.com> 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.

>> 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.


>> > WDYT?
>> >
>> > Radim
>> >
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> infinispan-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev