Quantcast

[infinispan-dev] Optional in listener events

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[infinispan-dev] Optional in listener events

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

WDYT?

Radim

--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

Sebastian Laskawiec
+1. I like the idea.

Thanks
Sebastian

On Fri, Aug 19, 2016 at 10:34 PM, Radim Vansa <[hidden email]> 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.

WDYT?

Radim

--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

Adrian Nistor
In reply to this post by Radim Vansa
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.

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.

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.
>
> WDYT?
>
> Radim
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

William Burns-3
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?

On Mon, Aug 22, 2016 at 4:31 AM Adrian Nistor <[hidden email]> 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.

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.

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.
>
> WDYT?
>
> Radim
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

Pedro Ruivo-2
If we can drop the pre-events, it would be possible to skip the
loading/wrapping of the previous value. The method DataContainer.put()
could return the previous value/metadata that could be used to trigger
the post-event.

However, it has a problem that I haven't solved it yet: if the value is
not in memory it will not be loaded (cache-stores / rebalance in progress).

Would it work for CQ?

Cheers,
Pedro

On 22-08-2016 19:15, 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?
>
> On Mon, Aug 22, 2016 at 4:31 AM Adrian Nistor <[hidden email]
> <mailto:[hidden email]>> 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.
>
>     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.
>
>     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.
>     >
>     > WDYT?
>     >
>     > Radim
>     >
>
>     _______________________________________________
>     infinispan-dev mailing list
>     [hidden email] <mailto:[hidden email]>
>     https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

Adrian Nistor
No. CQ absolutely needs the previous value.


On 08/24/2016 12:54 PM, Pedro Ruivo wrote:

> If we can drop the pre-events, it would be possible to skip the
> loading/wrapping of the previous value. The method DataContainer.put()
> could return the previous value/metadata that could be used to trigger
> the post-event.
>
> However, it has a problem that I haven't solved it yet: if the value is
> not in memory it will not be loaded (cache-stores / rebalance in progress).
>
> Would it work for CQ?
>
> Cheers,
> Pedro
>
> On 22-08-2016 19:15, 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?
>>
>> On Mon, Aug 22, 2016 at 4:31 AM Adrian Nistor <[hidden email]
>> <mailto:[hidden email]>> 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.
>>
>>      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.
>>
>>      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.
>>      >
>>      > WDYT?
>>      >
>>      > Radim
>>      >
>>
>>      _______________________________________________
>>      infinispan-dev mailing list
>>      [hidden email] <mailto:[hidden email]>
>>      https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

Adrian Nistor
In reply to this post by William Burns-3
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.

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?

On Mon, Aug 22, 2016 at 4:31 AM Adrian Nistor <[hidden email]> 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.

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.

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.
>
> WDYT?
>
> Radim
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev



_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

Dan Berindei
On Wed, Aug 24, 2016 at 4:14 PM, Adrian Nistor <[hidden email]> 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 <[hidden email]> 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
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [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 <[hidden email]> 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 <[hidden email]> 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
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

Adrian Nistor
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 <[hidden email]> 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 <[hidden email]> 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
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>>
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Optional in listener events

Emmanuel Bernard
Just so you see Adrian is a lone wolf, there is no way to implement
continuous query nor listeners whose function is essentially
accumulators if we don't have the previous value. This is an important
feature.

Topology change is a situation that can be detected and notified to
rebuild the accumulator / CQ explicitly. But not sending the previous
value arbitrarily is something the CQ engine cannot account for and is a
no go.

Of course, reliability during things like topology changes would be even
better.

Emmanuel

On Mon 2016-08-29 16:55, Adrian Nistor wrote:

> 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 <[hidden email]> 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 <[hidden email]> 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
> >>>> [hidden email]
> >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >>>
> >>> _______________________________________________
> >>> infinispan-dev mailing list
> >>> [hidden email]
> >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> infinispan-dev mailing list
> >>> [hidden email]
> >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >> _______________________________________________
> >> infinispan-dev mailing list
> >> [hidden email]
> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Loading...