Need to make enhacements to CacheListener API

[jason.greene]

Do to the recent changes we made with CacheListener behavior I need to refactor the POJO notification implementation. In order for me to deliver reliable detach and collection remove notifications, I have to do some advanced state tracking. To do this I need to know two things:

1. 
   
2. Correlate pre and post requests - Since concurrent requests can be delivered from different transactions/threads I might associate the wrong requests. So I need a tx id with every notification.
   
   
3. TX end notification - I need to track all changes of a certain type within the lifespan of a TX (since a single pojo operation is always multiple cache operations, and thus always in a transaction)
   
   
   -Jason

[manik]

1. Even with using a thread pool, notifications for each call or transaction are performed by the same thread (see Notifier.invokeQueuedNotifications()) - a single Runnable is used to loop thru all registered listeners, for all registered notification events. So you will get pre and post events in order, and not concurrently.

2. Hmm, this could be a useful callback - txStarted, txCommitted, txROlledBack (with a Transaction object passed in? Is this useful?) Can't help but feel we're reinventing something here though.

[jason.greene]

1. Even with using a thread pool, notifications for each call or transaction are performed by the same thread (see Notifier.invokeQueuedNotifications()) - a single Runnable is used to loop thru all registered listeners, for all registered notification events. So you will get pre and post events in order, and not concurrently.

Yes, but the listener instances are shared across all threads, so there could be two runnables running at the same time (unless the pool size is 1). Alternatively the pre and post phase could be combined to one method call with the before and after data.

2. Hmm, this could be a useful callback - txStarted, txCommitted, txROlledBack (with a Transaction object passed in? Is this useful?) Can't help but feel we're reinventing something here though.

Well currently events are only delivered on commit, so rollback wouldn't be useful, although start and commit would be.

-Jason

[jason.greene]

1. Even with using a thread pool, notifications for each call or transaction are performed by the same thread (see Notifier.invokeQueuedNotifications()) - a single Runnable is used to loop thru all registered listeners, for all registered notification events. So you will get pre and post events in order, and not concurrently.

Yes, but the listener instances are shared across all threads, so there could be two runnables running at the same time (unless the pool size is 1). Alternatively the pre and post phase could be combined to one method call with the before and after data.

Hmm, or did you mean using the thread-id for correlation? If so that would work (provided there is a tx start stop), but that ties the implementation of notification delivery to single thread semantics. Having a tx id or some kind of special change id would allow for future threading behavior changes. This is of course assuming that others using the the CacheListener API want to do similar things.

-Jason

[manik]

Ok, so now we're at CR2. Not the time to be changing APIs, but I understand that if we need it we need it. The callbacks in consideration are block/unblock/startTx/endTx, right?

I like Jason's approach to using annotations rather than the interface methods - makes things easier for people who don't want to implement everything.

What do people think? Can/should we:

1) Live with the current API
2) Add to the existing interface
3) Switch to an annotation based approach
4) Do both 2 and 3, providing users with both ways of implementing listeners?

I think if we can live with (1) I suggest we do that. Failing that, I'd go with (3), since we'd be breaking compat with CR2 anyway. Then (2) and finally (4) which I think doesn't add much value.

I'd like to do this for CR3, and will hold back CR3 until we have a clear decision on this.

[vincent.marquez]

Just my two cents, I see nothing wrong with the current API, maybe adding a AbstractPojoCacheListener to make life a little more convenient.

I do think CachedFieldInterceptor needs to be removed before the data is actually taken out of the cache, and not in a seperate thread if that is being considered. As it stands now, after a pojo is detached from a remote cache instance, it still tries to retrieve values from the cache, so you get a null value.

I threw a quick unit test up under http://jira.jboss.com/jira/browse/JBCACHE-713

[jason.greene]

"vincent.marquez" wrote:
Just my two cents, I see nothing wrong with the current API, maybe adding a AbstractPojoCacheListener to make life a little more convenient.

We are mainly talking about the core Cache Listener API, not the POJO API, although we would probably want it to be similar.

[jason.greene]

Here is a draft proposal for the changes. Please send feedback:

http://wiki.jboss.org/wiki/Wiki.jsp?page=JBossCacheListenerAPIProposal

[manik]

Looks good to me, how would the addListener() API on Cache look?

[brian.stansberry]

The wiki page mentions "Merge pre and post phase". I need the current (CR2) timing. Specifically, I need the pre callback for nodePassivated. Merging to me implies "post".

In general though, the annotation idea seems good. The interface approach has proven too fragile. And I know I've created a lot of little inner classes extending AbstractCacheListener just so I could implement one callback. That's pretty clunky when I think about it.

It will be slower, since reflection is involved. We'll probably have to organize listeners by callback so we can quickly invoke on the relevant ones, rather than iterating through each listener for every callback to check if it is interested.

I guess the addListener API just takes Object. Odd, but an alternative is to make CacheListener a marker interface. That's even more odd.