API Issues integrating 2.0.0.DR1 in HEAD

[brian.stansberry]

I'm starting to integrate 2.0.0.DR1 and here's what I've hit so far:

1) How do you set an Option? I thought there was an InvocationContext.currentContext() or something. Doesn't seem to be. Only thing I see is a getter/setter in CacheSPI, but my code shouldn't need CacheSPI.

2) It's not possible to learn anything about the cache loader configuration via the Cache interface, unless you're willing to parse an Element. We need to find a way to expose stuff. Specifically what's needed for session repl are:

a) is there a cache loader?
b) is it set for passivation?
c) is it shared?


Less critical:


3) Let's expose the TransactionManager as a Configuration property. In the session repl case, the cache is using special transaction manager. The session repl code needs to get it so it can control the tx. For other use cases, if the environment (like JBoss AS) supports it why not dependency inject the TM into the cache rather using a TransactionManagerLookup? TransactionManagerLookup becomes a fallback for cases where we can't dependency inject.

4) Can we restore Node.getChildrenNames()? Iterating through the child collection and calling getFqn().getName() on each node is a pain.

5) Why is Node.getChildren() a Collection and not a Set? I expect it's because Map.getValues() is a Collection, but properly this method should return Set.

6) The Node interface desribes various returned Collections as being immutable. From a glance at NodeImpl it looks like they aren't. Also, it would be good to be more clear about thread safety as immutable and thread safe aren't the same.

[brian.stansberry]

I've started working on #2 above. This work ties into what I think should be done for MC usage of the cache.

1) We already have configuration via DOM Elements and Properties of complex types like CacheLoaderManager. We need to add the ability to configure via MC construction of POJOs that represent all the configuration elements. The MC doesn't need to use Element/Properties to build up these POJOs.
2) Configuration becomes a composite object, with a contained CacheLoaderConfig, (new) BuddyReplicationConfig and (revised) EvictionConfig.
3) When the old Element/Properties style of configuration is used, all parsing of DOM elements/reading of Properties to populate the config objects should be done within the config objects themselves. E.g. pass an Element to the constructor of CacheLoaderConfig and let it parse it. CacheLoaderManager constructor gets passed the CacheLoaderConfig, not an Element. BuddyManager gets passed a BuddyReplicationConfig, not an Element.
4) The various config objects like CacheLoaderConfig are now exposed to user code via a getter in Configuration. This means their setter methods are also exposed, so if those methods are invoked after the cache is started we need to check if that's allowed. This is already done in Configuration; need to apply the same technique in the other config objects.

I've got a good start on this.

[manik]

1) How do you set an Option? I thought there was an InvocationContext.currentContext() or something. Doesn't seem to be. Only thing I see is a getter/setter in CacheSPI, but my code shouldn't need CacheSPI.

I did do InvocationContext.currentContext() for a while, but ran into problems when using multiple cache instances in the same JVM and thread.

You would now have to do CacheSPI.getInvocationContext(), although I see your point about your not needing to use this interface. It is a bit heavy for the Cache interface though. Any other thoughts, otherwise I fear it would just have to be in Cache.

[manik]

3) Let's expose the TransactionManager as a Configuration property. In the session repl case, the cache is using special transaction manager. The session repl code needs to get it so it can control the tx. For other use cases, if the environment (like JBoss AS) supports it why not dependency inject the TM into the cache rather using a TransactionManagerLookup? TransactionManagerLookup becomes a fallback for cases where we can't dependency inject.

* What callback do I need to provide for the DI from the MC?
* The TM can be retrieved using the CacheSPI, again perhaps this could be downgraded to be accessible from Cache. Accessing this from the Configuration doesn't make much sense since the Configuration should be a valid, testable and checkable object even when the cache is not running while the TM is a resource that should only be available when the cache is running.

4) Can we restore Node.getChildrenNames()? Iterating through the child collection and calling getFqn().getName() on each node is a pain.

I agree - will do.

5) Why is Node.getChildren() a Collection and not a Set? I expect it's because Map.getValues() is a Collection, but properly this method should return Set.

You guessed why. :-) Does it make more sense as a Set? I suppose so, since only one instance of each child would exist. I'd need to think of an efficient way to do this though, as iterating and creating a new set every time getChildren() is called is sub-optimal.

6) The Node interface desribes various returned Collections as being immutable. From a glance at NodeImpl it looks like they aren't. Also, it would be good to be more clear about thread safety as immutable and thread safe aren't the same.

Which ones aren't? I may have missed some. The point is not for thread safety, but so changes go through the Node or Cache API and hence the interceptor stack.

[manik]

1) We already have configuration via DOM Elements and Properties of complex types like CacheLoaderManager. We need to add the ability to configure via MC construction of POJOs that represent all the configuration elements. The MC doesn't need to use Element/Properties to build up these POJOs.

2) Configuration becomes a composite object, with a contained CacheLoaderConfig, (new) BuddyReplicationConfig and (revised) EvictionConfig.

+1.

3) When the old Element/Properties style of configuration is used, all parsing of DOM elements/reading of Properties to populate the config objects should be done within the config objects themselves. E.g. pass an Element to the constructor of CacheLoaderConfig and let it parse it. CacheLoaderManager constructor gets passed the CacheLoaderConfig, not an Element. BuddyManager gets passed a BuddyReplicationConfig, not an Element.
4) The various config objects like CacheLoaderConfig are now exposed to user code via a getter in Configuration. This means their setter methods are also exposed, so if those methods are invoked after the cache is started we need to check if that's allowed. This is already done in Configuration; need to apply the same technique in the other config objects.

+1 again. Yes, this does give us a lot of flexibility in how we configure complex structures like cache loaders, eviction policies, etc.

I've got a good start on this.

Cool!

This feedback really does help; I'm looking at cutting an Alpha once these changes are in. Since you already have a start on the config side of things, could I leave this to you?

I'm hoping to tag the alpha later this week, perhaps on Thu. Does this work for you?

[brian.stansberry]

You would now have to do CacheSPI.getInvocationContext(), although I see your point about your not needing to use this interface. It is a bit heavy for the Cache interface though. Any other thoughts, otherwise I fear it would just have to be in Cache.

Yeah, I think it needs to be in Cache.

Also, it wasn't clear to me how to get ahold of a CacheSPI. Smack me if you already explained a while back and I forgot. Remember that casting is not an option if you are working with a JMX proxy. (Plus it's nasty ;)

[brian.stansberry]

This feedback really does help; I'm looking at cutting an Alpha once these changes are in. Since you already have a start on the config side of things, could I leave this to you?

I'm hoping to tag the alpha later this week, perhaps on Thu. Does this work for you?

Sure, I'll do it. Thur may be tough, as I'm at a training. The concept leads to lots of details that need to be done; e.g. specific config POJOs for the various types of cache loaders.

[brian.stansberry]

* What callback do I need to provide for the DI from the MC?
* The TM can be retrieved using the CacheSPI, again perhaps this could be downgraded to be accessible from Cache. Accessing this from the Configuration doesn't make much sense since the Configuration should be a valid, testable and checkable object even when the cache is not running while the TM is a resource that should only be available when the cache is running.

I'd have to think about it if doing it through the configuration is no good (I agree w/ what you're saying about that.) What I've been thinking about w/ the MC is to have it build the config and then create the cache via the factory. I suppose once its done that it could inject the TM via a setTransactionManager method. But that sounds like a hack.

This isn't a critical issue; the session repl thing is a bit of an odd case and I've got a (hacky) workaround for it.

5) Why is Node.getChildren() a Collection and not a Set? I expect it's because Map.getValues() is a Collection, but properly this method should return Set.

You guessed why. :-) Does it make more sense as a Set? I suppose so, since only one instance of each child would exist. I'd need to think of an efficient way to do this though, as iterating and creating a new set every time getChildren() is called is sub-optimal.

Probably not worth it. I just mentioned it because I happened to notice it.

6) The Node interface desribes various returned Collections as being immutable. From a glance at NodeImpl it looks like they aren't. Also, it would be good to be more clear about thread safety as immutable and thread safe aren't the same.

Which ones aren't? I may have missed some. The point is not for thread safety, but so changes go through the Node or Cache API and hence the interceptor stack.

[brian.stansberry]

Sorry, last bit of last post got truncated...

6) The Node interface desribes various returned Collections as being immutable. From a glance at NodeImpl it looks like they aren't. Also, it would be good to be more clear about thread safety as immutable and thread safe aren't the same.

Which ones aren't? I may have missed some. The point is not for thread safety, but so changes go through the Node or Cache API and hence the interceptor stack.

Never mind re: not being immutable. I was looking at NodeImpl, which I see now doesn't actually implement Node. :-)

The concern I have re: thread safety is if a collection backed by the internal data structure of a node is exposed, you can get a ConcurrentModificationException just iterating over it. Not sure if that really happens. I saw NodeImpl.getDataKeys() exposes a collection backed by the data map. Haven't followed the code to see what happens to it. But, if that gets passed to a caller, any lock on the node could easily be released before the collection is even received by the caller (assume no tx). Thus easy to have another thread modify the data map via a put/remove while the first thread is iterating. Actually, as I write this, if what I'm describing is there, it's not a javadoc issue, it's something that needs to be changed.

[manik]

Also, it wasn't clear to me how to get ahold of a CacheSPI. Smack me if you already explained a while back and I forgot. Remember that casting is not an option if you are working with a JMX proxy. (Plus it's nasty ;)

Casting is nasty and unsupported. The only correct way to get a hold of the CacheSPI is to write an Interceptor or CacheLoader, and the CacheSPI will be injected into your code.

Remember, this is not a client API. :-)

[manik]

The concern I have re: thread safety is if a collection backed by the internal data structure of a node is exposed, you can get a ConcurrentModificationException just iterating over it. Not sure if that really happens. I saw NodeImpl.getDataKeys() exposes a collection backed by the data map. Haven't followed the code to see what happens to it. But, if that gets passed to a caller, any lock on the node could easily be released before the collection is even received by the caller (assume no tx). Thus easy to have another thread modify the data map via a put/remove while the first thread is iterating. Actually, as I write this, if what I'm describing is there, it's not a javadoc issue, it's something that needs to be changed.

getKeys() and getData() currently return unmodifiable maps based on the internal data structures in a node. I see your point about CMEs. This would have to be a simple copy I suppose, but I'm not too keen on the performance overhead of this copy. It seems to be the most 'correct' thing to do though.

[brian.stansberry]

How about using ConcurrentReaderHashMap for the internal maps?

[manik]

I'd need to double-check, but I believe they already are ...