[infinispan-dev] Optional in listener events

[Radim Vansa]

On 08/24/2016 04:32 PM, Dan Berindei wrote:
> 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.

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

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 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
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


-- 
Radim Vansa <rvansa at redhat.com>
JBoss Performance Team