I think I'm leaning towards 3. I would favour 2 if all listeners were already in place for Functional API, since I think down the line we'd want to offer 3 map-like APIs: ConcurrentMap, JCache and Functional Map. Hence phasing out Cache as an end-user API... Since we're not there yet, I'd vote for option 3 below. Cheers, -- Galder Zamarreño Infinispan, Red Hat > On 5 Oct 2015, at 07:34, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > Hi guys > > We have two different listener types for cache entry creation > (@CacheEntryCreated) and modification (of an already-existing entry, > @CacheEntryModified). However, PutMapCommand and > PutKeyValueCommand/IGNORE_RETURN_VALUES do not read the previous value > from loaders and/or remote nodes, so we sometimes notify the > @CacheEntryCreated listeners instead of the @CacheEntryModified > listeners. > > PutMapCommand has a "workaround" for this: it also notifies the > @CacheEntryModified listener, regardless of whether it found the entry > or not. I'd like to change this [1] and make PutMapCommand and > PutKeyValueCommand behave the same way. > > These are the options I'm considering: > > 1. Replicate the PutKeyValueCommand behaviour, and document that we > may sometimes notify the @CacheEntryCreated listener even though the > entry already exists. > It would be the simplest to implement (in fact I already have a > patch), but it doesn't feel right. > > 2. Always read the previous value from loaders and/or remote nodes > when a @CacheEntryCreated/Modified event listener is registered. > This would give us the correct behaviour, at the expense of performance. > > 3. Same as option 2, but also add a @CacheEntryWritten listener type, > which only receives the new value and is notified regardless of > whether the entry was created or modified. > This would give users a choice: if they don't care about the previous > value, the cache will be just as fast as usual, but if they need the > previous value, they need to accept a slowdown. > > 4. Add the @CacheEntryWritten listener type, but only notify these > listeners instead of the @CacheEntryCreated/Modified listeners for > cache.putAll() and cache.withFlags(IGNORE_RETURN_VALUES).put(). > This is the option Galder chose for the functional API, but the > difference between write-only and read-write operations is a lot > clearer there, so I'm not convinced it's ok for the ConcurrentMap API. > > > Personally I would choose option 3, because it would be mostly > backwards-compatible: old code would only need to change if existing > listeners are slowing down the cache. > > Cheers > Dan > > [1] https://issues.jboss.org/browse/ISPN-5752 > _______________________________________________ > 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

I'm not sure about including removals/invalidations/expiration,

because there would be no way to say "I want to be notified on creation and modification, but no removals". On the other hand, adding

3 more methods delegating to the same implementation, while not pretty, does allow you to listen to all changes.

Or are you thinking that the 3 additional listeners would add significant overhead when clustered?

Cheers Dan On Thu, Oct 8, 2015 at 6:06 PM, William Burns <mudokonman(a)gmail.com&gt; wrote: > I also like #3. However I wonder if we want to expand it to include cache > removal/expiration in this new event as well. This would simplify a user > listening for data being changed in only a given listener and it could be > clustered. > > - Will > > On Thu, Oct 8, 2015 at 6:38 AM Galder Zamarreño <galder(a)redhat.com&gt; wrote: >> >> I think I'm leaning towards 3. >> >> I would favour 2 if all listeners were already in place for Functional >> API, since I think down the line we'd want to offer 3 map-like APIs: >> ConcurrentMap, JCache and Functional Map. Hence phasing out Cache as an >> end-user API... Since we're not there yet, I'd vote for option 3 below. >> >> Cheers, >> -- >> Galder Zamarreño >> Infinispan, Red Hat >> >> > On 5 Oct 2015, at 07:34, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> > >> > Hi guys >> > >> > We have two different listener types for cache entry creation >> > (@CacheEntryCreated) and modification (of an already-existing entry, >> > @CacheEntryModified). However, PutMapCommand and >> > PutKeyValueCommand/IGNORE_RETURN_VALUES do not read the previous value >> > from loaders and/or remote nodes, so we sometimes notify the >> > @CacheEntryCreated listeners instead of the @CacheEntryModified >> > listeners. >> > >> > PutMapCommand has a "workaround" for this: it also notifies the >> > @CacheEntryModified listener, regardless of whether it found the entry >> > or not. I'd like to change this [1] and make PutMapCommand and >> > PutKeyValueCommand behave the same way. >> > >> > These are the options I'm considering: >> > >> > 1. Replicate the PutKeyValueCommand behaviour, and document that we >> > may sometimes notify the @CacheEntryCreated listener even though the >> > entry already exists. >> > It would be the simplest to implement (in fact I already have a >> > patch), but it doesn't feel right. >> > >> > 2. Always read the previous value from loaders and/or remote nodes >> > when a @CacheEntryCreated/Modified event listener is registered. >> > This would give us the correct behaviour, at the expense of performance. >> > >> > 3. Same as option 2, but also add a @CacheEntryWritten listener type, >> > which only receives the new value and is notified regardless of >> > whether the entry was created or modified. >> > This would give users a choice: if they don't care about the previous >> > value, the cache will be just as fast as usual, but if they need the >> > previous value, they need to accept a slowdown. >> > >> > 4. Add the @CacheEntryWritten listener type, but only notify these >> > listeners instead of the @CacheEntryCreated/Modified listeners for >> > cache.putAll() and cache.withFlags(IGNORE_RETURN_VALUES).put(). >> > This is the option Galder chose for the functional API, but the >> > difference between write-only and read-write operations is a lot >> > clearer there, so I'm not convinced it's ok for the ConcurrentMap API. >> > >> > >> > Personally I would choose option 3, because it would be mostly >> > backwards-compatible: old code would only need to change if existing >> > listeners are slowing down the cache. >> > >> > Cheers >> > Dan >> > >> > [1] https://issues.jboss.org/browse/ISPN-5752 >> > _______________________________________________ >> > 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

On Fri, Oct 9, 2015 at 4:39 PM, William Burns <mudokonman(a)gmail.com&gt; wrote: > > > On Thu, Oct 8, 2015 at 12:39 PM Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> >> I'm not sure about including removals/invalidations/expiration, > > > Invalidations to me don't quite fit, since it should be passivated in that > case. Passivations have a different listener, I didn't include @CacheEntryPassivated here :) Perhaps invalidation doesn't fit, because it's used to remove stale entries either after a rebalance, or after a write (for L1 entries, or in invalidation mode). But then I would say expiration also doesn't fit, because the all the others are direct actions by the user, and expiration happens in the background. > >> >> because there would be no way to say "I want to be notified on >> creation and modification, but no removals". On the other hand, adding > > > We could always add a parameter to the new annotation to say if you don't > care about removals maybe? Yes, that should work. > >> >> 3 more methods delegating to the same implementation, while not >> pretty, does allow you to listen to all changes. > > > Do we need 3 methods? Yes I think it would be nice for people. I'm assuming the listener methods are checked to receive the right event types. If we allow supertypes (and make CacheEntryWrittenEvent a supertype of the others) it could be just 1 method with 4 annotations. > >> >> >> Or are you thinking that the 3 additional listeners would add >> significant overhead when clustered? > > > I was thinking it would be 1 listener. CacheNotifierImpl could raise the > new event in addition to the existing ones. Right, if we include the removals in this annotation there would be just one listener. But would it be faster than having 4 listeners, one for create/update, and one each for remove/invalidation/expiration? Cheers Dan

-- Galder Zamarreño Infinispan, Red Hat > On 14 Oct 2015, at 16:43, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > There is one more thing to consider: the interface for acting on the > previous value of an event is very clunky even today. It basically > requires a thread-local to keep track of the previous value from the > pre=true invocation to the pre=false invocation, and even that won't > work once the reactive interceptor stack lands (so that pre and post > events are triggered on different threads). > > So I'm starting to think we should go with option 1 for now, and start > a bigger redesign of the cache notifications for 9.0: > * Remove the pre=true events > * Add explicit event properties for the previous value/metadata Why redesign cache notifications? As I mentioned in a previous reply, I see Cache and cache notifications being phased out in favour of: JCache (and its events), ConcurrentMap and Functional Map (and its events) :|.

So, no need to redesign Cache notifications IMO :|

> > Without backwards compatibility requirements, we could even add a > skipPreviousValue parameter to all listener annotations -- except for > @CacheEntryCreated, I guess -- making the new listener type > superfluous. > > Cheers > Dan > > > On Mon, Oct 12, 2015 at 11:02 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> On Fri, Oct 9, 2015 at 4:39 PM, William Burns <mudokonman(a)gmail.com&gt; wrote: >>> >>> >>> On Thu, Oct 8, 2015 at 12:39 PM Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>>> >>>> I'm not sure about including removals/invalidations/expiration, >>> >>> >>> Invalidations to me don't quite fit, since it should be passivated in that >>> case. >> >> Passivations have a different listener, I didn't include >> @CacheEntryPassivated here :) >> >> Perhaps invalidation doesn't fit, because it's used to remove stale >> entries either after a rebalance, or after a write (for L1 entries, or >> in invalidation mode). >> >> But then I would say expiration also doesn't fit, because the all the >> others are direct actions by the user, and expiration happens in the >> background. >> >>> >>>> >>>> because there would be no way to say "I want to be notified on >>>> creation and modification, but no removals". On the other hand, adding >>> >>> >>> We could always add a parameter to the new annotation to say if you don't >>> care about removals maybe? >> >> Yes, that should work. >> >>> >>>> >>>> 3 more methods delegating to the same implementation, while not >>>> pretty, does allow you to listen to all changes. >>> >>> >>> Do we need 3 methods? Yes I think it would be nice for people. >> >> I'm assuming the listener methods are checked to receive the right >> event types. If we allow supertypes (and make CacheEntryWrittenEvent a >> supertype of the others) it could be just 1 method with 4 annotations. >> >>> >>>> >>>> >>>> Or are you thinking that the 3 additional listeners would add >>>> significant overhead when clustered? >>> >>> >>> I was thinking it would be 1 listener. CacheNotifierImpl could raise the >>> new event in addition to the existing ones. >> >> Right, if we include the removals in this annotation there would be >> just one listener. But would it be faster than having 4 listeners, one >> for create/update, and one each for remove/invalidation/expiration? >> >> Cheers >> Dan > _______________________________________________ > 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

-- Galder Zamarreño Infinispan, Red Hat > On 20 Oct 2015, at 15:05, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > On Mon, Oct 19, 2015 at 5:34 PM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >> >> >> -- >> Galder Zamarreño >> Infinispan, Red Hat >> >>> On 14 Oct 2015, at 16:43, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>> >>> There is one more thing to consider: the interface for acting on the >>> previous value of an event is very clunky even today. It basically >>> requires a thread-local to keep track of the previous value from the >>> pre=true invocation to the pre=false invocation, and even that won't >>> work once the reactive interceptor stack lands (so that pre and post >>> events are triggered on different threads). >>> >>> So I'm starting to think we should go with option 1 for now, and start >>> a bigger redesign of the cache notifications for 9.0: >>> * Remove the pre=true events >>> * Add explicit event properties for the previous value/metadata >> >> Why redesign cache notifications? As I mentioned in a previous reply, I see Cache and cache notifications being phased out in favour of: JCache (and its events), ConcurrentMap and Functional Map (and its events) :|. > > How is the ConcurrentMap API different from the Cache API? Cache is ConcurrentMap + a mixed bag of other things which includes all sorts of things, from put operations that take extra parameters, to ways to retrieve the Batch container :| ConcurrentMap is what is given to use by the JDK whereas the rest of Cache API is what we want to have, or legacy stuff we've had since day 1. Where possible, we should promote users to use standard APIs, so that's ConcurrentMap (as-is, no Cache) and JCache. If the users find limitations on those, we should try to fill those gaps with Functional Map. It's true that today, using ConcurrentMap is done by getting a Cache, which IMO is really confusing in how we expose our APIs. So, yeah, if people want to use ConcurrentMap, they need Cache, but that should not be the case.

> >> >> So, no need to redesign Cache notifications IMO :| > > I was assuming JCache listeners are implemented on top of the Cache > listeners, so they would need fixes in the Cache listeners to work > properly. ^ Yeah, currently that is the case.

> We could implement the JCache listeners directly in 9.0 and remove the > Cache listeners, I guess. But looking at the JCache API it looks like > their listeners need the previous value for all writes, unlike the > FunctionalMap listeners, so I think there will still be a need for a > lower-level implementation. Depends. JCache API impl could decide, based on whether any listeners are attached, whether those write-only operations should become read-write operations because listeners attach demand it, as opposed for the actual function requiring to read previous value.