-
1. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
mircea.markus Jan 19, 2011 4:34 AM (in response to mpokorny)Comparing the two approaches, I like yours more, especially for the last reason.
CONS
- Breaking change if one wants to Cache if an addListener(CacheListenerInterface) is added.
agreed. People extending from the Abstract listener won't be affected though.
-
2. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
mpokorny Jan 19, 2011 5:01 AM (in response to mircea.markus)Thinking a bit more I appreciate that one uber interface w/ all the listener events is not desirable because it means there will be listeners registered for all the events that really do nothing. I tehrefore, so i however propose an interface for each event type.
A top level interface is needed so Cache has only one addListener event.
interface InfinispanListener{
// no methods
};
Cache {
addListener(InfinispanListener); // still needs to verify that InfinispanListener is one of the supported types throwing an ex if not.
// this means anyone who extends InfinispanListener instead of one of the provided sub interfaces fails.
}
interface CacheValueCreatedListener extends InfinispanListener {
void onCreate( CacheEntryCreatedEvent e );
}
// repeat for all other events...
-
3. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
mpokorny Jan 19, 2011 5:02 AM (in response to mpokorny)Should i create a jira as an improvement ???
-
4. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
mircea.markus Jan 19, 2011 5:04 AM (in response to mpokorny)Let's here some more opinions. This is the way things were implemented in JBossCache at one point, so just wondering what other drawbacks this approach might have.
-
5. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
manik Jan 19, 2011 7:28 AM (in response to mpokorny)Right, this is the main reason for the annotation-based approach. So you don't have lots of listener interfaces (messy to code to), and you don't have one (expensive) uber-interface with mostly no-op implementations.
E.g., it is cleaner, easier to do:
@Listener class MyListener { @CacheEntryCreated public void entryCreated( ... ) { ... } @CacheEntryModified public void entryModified( ... ) { ... } @CacheEntryRemoved public void entryVisited( ... ) { ... } @CacheEntryVisited public void entryRemoved( ... ) { ... } }
rather than
class MyListener implements CacheEntryCreatedListener, CacheEntryModifiedListener, CacheEntryRemovedListener, CacheEntryVisitedListener { public void entryCreated( ... ) { ... } public void entryModified( ... ) { ... } public void entryVisited( ... ) { ... } public void entryRemoved( ... ) { ... } }
-
6. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
mircea.markus Jan 19, 2011 7:35 AM (in response to manik)It's about the same amount of code though, and the second approach is type safe - which is good!
Also there can be an CacheListener (implementing all other listeners) and also an abstract CacheListenerSupport - that would make things easier.
-
7. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
manik Jan 19, 2011 7:55 AM (in response to mircea.markus)A CacheListener and abstract CacheListenerSupport (with no-op impls) is a bad idea. This is why we moved to annotations in the first place. These things placed a huge performance penalty on cache operations since there is no way to tell if a listener is really interested in the callback or not.
-
8. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
mpokorny Jan 19, 2011 8:03 AM (in response to manik)But using annotations means lsiteners are dispatched using a Method.invoke() which is more costly than a regular Interface.method invoke.
-
9. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
manik Jan 19, 2011 10:29 AM (in response to mpokorny)On modern JVMs this isn't that great. Most of the expense is traversing class definitions to find methods, but this happens only once when the listener is registered, and is subsequently cached. The method invocation via reflection isn't that expensive.
And it is many times cheaper than a lot of unnecessary invocations to no-op method impls.
-
10. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
sudheerk84 Jan 25, 2011 4:45 AM (in response to manik)Hi All,
Personally i also like the annotation approach. But i am currently facing a drawback by using annotations as agaist API's.
I am trying to writing a cache layer which hides the exact cache implementation. Currently Infinispan supprts my requirements but in teh future we may have usescases for which we may need to use other cachinh solution. For this reason all our components use teh cache API(com.mycompany.cache.*) layer i have written instead of directly depeneding on Jboss api's.
But with annotations this is not possible. My listeners are directly dependent on JBOSS APIs. Soem day when i change teh cache implentation behing i needs to manually go to these lisiteners and change code.
If there was an API to register for cache events i would haev written a wrapper liek com.mycmpany.listenere.register() and unregister() methods which would hide teh provider used behind.
-
11. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
mircea.markus Jan 25, 2011 7:10 AM (in response to sudheerk84)I am trying to writing a cache layer which hides the exact cache implementation. Currently Infinispan supprts my requirements but in teh future we may have usescases for which we may need to use other cachinh solution. For this reason all our components use teh cache API(com.mycompany.cache.*) layer i have written instead of directly depeneding on Jboss api's.
Alternatively you can stick with a java.util.Map interface for that, a-la JSR-107. AFAIK most important caching libraries expose the Map API for caching.
But with annotations this is not possible. My listeners are directly dependent on JBOSS APIs. Soem day when i change teh cache implentation behing i needs to manually go to these lisiteners and change code.
That would still be the case if you used direct interfaces.
If there was an API to register for cache events i would haev written a wrapper liek com.mycmpany.listenere.register() and unregister() methods which would hide teh provider used behind.
I think that's a good approach. You can write your listener implementation in a vendor neutral manner, then you can have a listener factory (as described) that would register some vendor specific listeners (adaptor-GOF) that would delegate calls to your vendor neutral listener implementation.
HTH,
Mircea
-
12. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
sudheerk84 Jan 25, 2011 7:28 AM (in response to mircea.markus)I have a slight confusinon the third point you mentioned.
Now by adding annotation to the listeners my class has to import org.infinispan.notifications.Listener; [i am fine with my api layer importing jboss classes but would like to avoid teh modules using my api layer to avoid using jboss jars]
I was just trying to avoid my custom classes directly dependencing on Jboss jars , but this is not possible as they have annotation class dependency.
-
13. Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
mircea.markus Jan 25, 2011 8:12 AM (in response to sudheerk84)I was just trying to avoid my custom classes directly dependencing on Jboss jars , but this is not possible as they have annotation class dependency.
My understanding is that you want to write the listener code in a portable manner. A design idea for this would be:
- write your listener logic in an vendor independent class, let's say AListener. A does not depend on ISPN classes or anything
- write a class IspnAListenerAdapert that is ISPN-dependent listener implementation. What this class does is, on each notification, unwraps the call and delegates it to AListener's method. This is basically the Adapter pattern described in GOF.
- use an factory method to register IspnALstenerAdapter to the Ispn instance. Perhaps the same factory method you use for obtaining the cache handler
- if you need to try a new vendor, just re-implement another VendorAListenerAdapter - this should be quite easy as the adapter itself does not have any logic ...
-
14. Re: Improvement suggestion: Use interface for all Listener events rather than annotation tagging...
sudheerk84 Jan 25, 2011 8:27 AM (in response to mircea.markus)Now i get what you meant. So you are suggesting to write a wrapper over the each listener class which i was trying to avoid. I wanted to write a single adaptor which will hide the registration and unregistration.
I am not sure writing one adaptor class per listerner is a good idea. I would have prefreed something like below
Assuming there was a interface org.infinispan.notifications.InfinipanListener and all listeners shoudl implement/extend this class.
I would liek to register my listeners through API - cache.addListerForAddevent( abcListener) or cache.addListenerForRemoveEvent( xyzListener)
Both abcListener and xyzListener will implement org.infinispan.notifications.InfinipanListener( not directly, instead through a wrapper com.mycompany.notofication.Listener so that abcListener and xyzLisrtener will not depend(import classes) on infinispam instead on my wrapper)
In this model I need to have single adaptor with 2 methods - addListerForAddevent and addListenerForRemoveEvent which internally registers
listeners with the infinispam registry.
The advanatge i get here is when i move to a new cache implementation , i dont need to go and change each listenerr class(Which was the main reason for wrting a wrapper layer) and instead change in a single class.
I just mentioned in this thread because this was talking about using Annotations Vs interface for registration/notification.
Am i getting anything wrong here ?