CR3 Listener

[fredrikj]

I just upgraded to CR3 and found out that the CacheListener interface has been removed in favor for the annotationbased listener.

Some gripes:

1. Wouldn't this be considered a major api change(?) and as such should not go in between two cr releases?

2. I understand that the benefit is that you can choose to declare only those listener methods you are interested of, thus reducing the amount of empty boilerplate listener methods. However, the downpart is that we are losing type safety here and I personally do not consider this a good bargain.

From the documentation we can read:

Methods annotated as such need to be public, have a void return type, and accept a single parameter of type org.jboss.cache.notifications.event.Event or one of it's subtypes.

Now, isn't this exactly why we have interfaces? An interface enforces signature and types. If we fail to honor the interface we get a nice compilation error, with the annotation based model we will get runtime checking and runtime errors.

If we look at the list of available annotations here http://labs.jboss.com/file-access/default/members/jbosscache/freezone/docs/2.0.0.CR3/JBossCache-UserGuide/en/html_single/index.html#api.listener we can see that the input ot the methods differ. Now I have to switch between my IDE and the documentation to lookup the annotation and what the proper signature is. Should I fail to comply, my IDE will not tell me since there is no compile-time checking.


Furthermore, since there is no longer a CacheListener interface, I cannot use that interface for listener-registration in layers created on top of the cache, forcing me either into allowing Object or creating my own Listener interface which will allow me at least some typesafety. But in the end I will never really know until the cache is started and every possible listener has been registered to the cache, right?

Generally I'm not opposed annotations. I think they do fill a purpose, but I can't seem understand the reasons for this particular design decision.

[jason.greene]

"FredrikJ" wrote:

1. Wouldn't this be considered a major api change(?) and as such should not go in between two cr releases?

Unfortunately we ran into unexpected implementation problems that the current API could not address. We had to choose between complicating the system by introducing a secondary API, or breaking API compatibility and fixing the extensibility problems that lead to this issue in the first place. We decided that overall it was better to get a solid, extensible, and easy to use API with well defined behavior in the GA. Had this issue been discovered after GA, we would have not taken this option.

Before making this change, we also posted a detailed proposal and asked for feedback from everyone.

2. I understand that the benefit is that you can choose to declare only those listener methods you are interested of, thus reducing the amount of empty boilerplate listener methods. However, the downpart is that we are losing type safety here and I personally do not consider this a good bargain.

That is one of the benefits, although the main reason is to allow easy extensibility. In this model we can add a new notification type in a future release, and it will not break existing listeners.

Now, isn't this exactly why we have interfaces? An interface enforces signature and types. If we fail to honor the interface we get a nice compilation error, with the annotation based model we will get runtime checking and runtime errors.

As mentioned above, the interface in this particular use case is not the best fit. It can't be extended, and it forces you to choose between implementing a bunch of methods you don't care about, or polluting your inheritance tree with a JBoss Cache class. The annotation model lets you choose whatever class design fits your application the best.

You do lose compile time checking, although the API is very simple and predictable. The run-time error occurs the first time you register a listener, which should be close to immediate.

Also, I should note that this is not a unique concept. EJB3 and DI frameworks use similar concepts.

If we look at the list of available annotations here http://labs.jboss.com/file-access/default/members/jbosscache/freezone/docs/2.0.0.CR3/JBossCache-UserGuide/en/html_single/index.html#api.listener we can see that the input ot the methods differ. Now I have to switch between my IDE and the documentation to lookup the annotation and what the proper signature is. Should I fail to comply, my IDE will not tell me since there is no compile-time checking.

If you point your IDE to the JBossCache javadocs, you will be able to pull up the info without leaving it, since we put docs on all the annotations. Also there is an easy convention, all notification methods take a single parameter, and its type is the annotation name + "Event".

i.e. @NodeModified public void doSomething(NodeModifiedEvent event) {}

Furthermore, since there is no longer a CacheListener interface, I cannot use that interface for listener-registration in layers created on top of the cache, forcing me either into allowing Object or creating my own Listener interface which will allow me at least some typesafety. But in the end I will never really know until the cache is started and every possible listener has been registered to the cache, right?

Yes, although typically this occurs right away. Also usually one would test their listener before going into production, as compile time safety does not promise run-time correctness.

Generally I'm not opposed annotations. I think they do fill a purpose, but I can't seem understand the reasons for this particular design decision.

I appreciate your feedback, and I understand your concerns. We are still open to any ideas you have that can make this better.

Here is the trail for all the design discussions that lead to these changes:
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=110525
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=111094
http://wiki.jboss.org/wiki/Wiki.jsp?page=JBossCacheListenerAPIProposal
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=110911

-Jason

[aditsu]

I was going to post something along the same lines as FredrikJ, so I'm glad to see this thread.

Could we perhaps have an adapter (abstract listener class) with annotated callback methods, that we can extend for defining listeners with compile-time checking?

Also, are the listener callbacks currently fired synchronously as part of the transaction, therefore blocking the transaction till they are executed? Will that change in the future?

Thanks
Adrian

[manik]

"aditsu" wrote:
I was going to post something along the same lines as FredrikJ, so I'm glad to see this thread.

Could we perhaps have an adapter (abstract listener class) with annotated callback methods, that we can extend for defining listeners with compile-time checking?

Something we did consider, but such an adapter would lose some the benefits of the annotation based approach. Performance on callbacks you are not interested in. Empty impl's of callbacks still get called, but a callback isn't annotated, it doesn't.

"aditsu" wrote:

Also, are the listener callbacks currently fired synchronously as part of the transaction, therefore blocking the transaction till they are executed? Will that change in the future?

Yes, they are synchronous (see updated javadocs on the annotation in CVS HEAD). I don't expect this to change in the future.

[manik]

"aditsu" wrote:
Could we perhaps have an adapter (abstract listener class) with annotated callback methods, that we can extend for defining listeners with compile-time checking?

And for compile time checking to make any sense, we'd have to change the API from cache.addListener(Object) to cache.addListener(AbstractListener) which means that no one would have the flexibility of only using the annotation-based approach.

The other thing we considered was a combination interface/annotation approach, where we'd use an empty CacheListener interface instead of the class-level @CacheListener annotation, followed by method level annotations. We thought that this was a pretty pointless exercise since the interface, being empty, didn't really buy anything and would be almost as pointless as Serializable.

[supi]

"FredrikJ" wrote:

1. Wouldn't this be considered a major api change(?) and as such should not go in between two cr releases?

I totally agree. We will have to go into production with a patched cr2.

[manik]

Err, I wouldn't recommend anyone going into production with a non-production release of a product.