See below my comments:
On Dec 30, 2010, at 7:05 AM, Eduardo Martins wrote:
Hi all, just completed first iteration of Mobicents Cluster
Framework
2.x, which includes impl using first alpha release of Infinispan 5. We
have everything we had in jboss cache working, it's a dumb down but
higher level framework, with stuff like Fault Tolerant timers, which
is then reuse in whole Mobicents platform to provide cluster related
features. I believe we now already use a lot of stuff from Infinispan
rich feature set, so I guess it's good timing to give some feedback,
report some minor issues, and clear some doubts.
1) Marshallers
Generally I'm "just OK" with the current API, the fact it
Externalizers depends on Annotations makes impossible for a higher
level framework to allow it's clients to plug their own kind of
Externalizers, without exposing Infinispan classes. In concrete, our
framework exposes its own Externalizer model, but to use them we need
to wrap the instances of classes these handle, in classes bound to
Infinispan Externalizers, which when invoked by Infinispan then lookup
our "own" Externalizers, that is, 2 Externalizers lookups per call. If
there was no annotations required we could wrap instead our clients
Externalizers and plug them into Infinispan, this would mean a single
Externalizer lookup. By the way, since the Externalizer now uses
generics the Annotations values look bad, specially since it's
possible to introduce errors that can compile.
First of all, the current API that you use in 5.0.0.ALPHA1 is not yet set in stone, and so
feedback like this is invaluable to get it right for ALPHA2 or BETA.
Externalizer is a concept that was borrowed from JBoss Marshalling that was squashed to
the bare minimum needed by Infinispan. Unless you plan to provide alternative Externalizer
implementations based in another framework (I don't see this really doable since the
way they're instantiated and used is very particular), I don't see the need to
abstract this as hugely important, although I can understand your leak worries.
Personally I think annotations fit perfectly for defining the classes it externalizers and
optionally the ids. However, a different avenue could be taken here which is adding
getTypeClasses()/getTypeClassNames()/getId() methods to Externalizer interface, but
I'm not keen on doing that since IMO pollutes a very clean Externalizer interface.
Another possibility could be to provide a sub-interface of Externalizer where those
methods are added and give people the freedom to decide how to feed in this metadata,
either via API or annotations. However, this could be adding complexity that might confuse
Externalizer providers.
Not sure I understand your 2 lookup call problem though. Maybe you can clarify this
further with the proposal I make above and how it would vary?
And I don't understand the errors that compile either. Please provide an example to
understand this better.
Another issue, in this case I consider it of minor importance due to
low level of Externalizers concept, is the lack of inheritance of
Externalizers, for instance we extend DefaultConsistentHash, instead
of extending its Externalizer I had to make an Externalizer from
scratch, and copy of DefaultConsistenHash's Externalizer code. This is
messy to manage, if the code on Infinispan change we will always have
to check if the code we copied is still valid.
Hmmm, can't you use delegation instead of inheritance? Delegation is generally
considered to be cleaner than inheritance. For example, see
org.infinispan.transaction.xa.DldGlobalTransaction and
org.infinispan.transaction.xa.GlobalTransaction. DldGlobalTransaction extends
GlobalTransaction but DldGlobalTransaction's Externalizer does not extend
GlobalTransaction's. Instead, DldGlobalTransaction's Externalizer takes
GlobalTransaction's Externalizer in the constructor and then delegates accordingly.
This avoids any code duplication.
Btw, out of curiosity, why finding yourself needing to extend the DefaultConsistentHash
externalizer?
2) CacheContainer vs CacheManager
The API is now deprecating the CacheManager, replacing with
CacheContainer, but some functionality is missing, for instance
CacheContainer is not a Listenable, thus no option to register
Listeners, unless unsafe casting of the instance exposed by the Cache
or a ref is available to the original CacheManager. Related matter,
IMHO a Cache listener should be able to listen to CacheManager events,
becoming global listeners.
Did you look at EmbeddedCacheManager? That's what you should be coding against that
allows you to add/remove listeners. You should not use CacheManager at all.
Wrt listener, what are you trying to say? That if a listener is registered to a Cache,
then it should receive CacheManager notifications if it has methods annotated accordingly,
i.e. @CacheStopped?
3) Configuration stuff
Generally I think the Configuration and GlobalConfiguration could be
simplified a bit, I found myself several times looking at the impl
code to understand how to achieve some configurations.
Configurations such as?
Better
documentation wouldn't hurt too, it's great to have a complete
reference, but the configuration samples are not ok, one is basically
empty, the other has all possible stuff, very unrealistic, would be
better to have reusable examples for each mode, with then
recommendations on how to improve these.
Which configuration samples do you refer to? The ones in the testsuite?
I suppose the reusable examples that you look for might be partially available via
TestCacheManagerFactory except that these are oriented at testing (i.e. make sure each
testsuite thread uses a different mcast address). Maybe we should extract these out and
make more easily available.
Infinispan 5 introduces a new global configuration setter to provide
an instance, with same method name as the one to provide the class
name. I believe one is enough, and to be more friendly with
Microcontainer and similar frameworks I would choose the one to set
the instance.
Which configuration setter are you referring to? It's worth mentioning that
GlobalConfiguration mixes both JAXB method requirements and programmatic configuration
needs, so maybe that's what's confusing things.
4) AtomicMap doubt
I read in the Infinispan blog that AtomicMap provides colocation of
all entries, is that idea outdated? If not we may need a way to turn
that off :) For instance would not that mean the Tree API does not
works well with distribution mode? I apologize in advance if I'm
missing something, but if AtomicMap defines colocation, AtomicMap is
good for the node's data map, but not for the node's childs fqns.
Shouldn't each child fqn be freely distributed, being colocated
instead with the related node cache entry and data (atomic)map? Our
impl is kind of an "hybrid" of the Tree API, allows cache entries
references (similar to childs) but no data map, and the storage of
references through AtomicMap in same way as Tree API worries me.
Please clarify.
Each FQN has underneath an AtomicMap, so each you won't find yourself finding k,v
pairs belonging to a particular Fqn in different nodes.
We make no guarantees wrt child fqn nodes. So, just cos FQN B is child of FQN A, it does
not mean that the atomic map of B will be in same node as atomic map of B.
5) Minor issues found
See these a lot, forgotten info logging?
03:39:06,603 INFO [TransactionLoggerImpl] Starting transaction logging
03:39:06,623 INFO [TransactionLoggerImpl] Stopping transaction logging
Most likely:
https://issues.jboss.org/browse/ISPN-852
MBean registration tries twice the same MBean, the second time fails
and prints log (no harm besides that, the process continues without
failures):
03:39:06,395 INFO [ComponentsJmxRegistration] Could not register
object with name:
org.infinispan:type=Cache,name="___defaultcache(dist_sync)",manager="DefaultCacheManager",component=Cache
That should not happen:
https://issues.jboss.org/browse/ISPN-853
6) Final thoughts
Kind of feel bad to provide all these negative stuff in a single mail,
took me an hour to write it, but don't get me wrong, I really enjoy
Infinispan, it's a great improvement. I'm really excited to have it
plugged in AS7 (any plan on this?) and then migrate our platform to
this new cluster framework. I expect a big performance improvement, on
something already pretty fast, and much less memory usage, our
Infinispan impl "feels" very fine grained and optimized in all points
of view. Of course, the distribution mode is the cherry on top of the
cake, hello true scalability.
I don't think it's negative stuff at all. On the contrary, I think it's really
valuable feedback from another Infinispan user :)
AS7 will of course have Infinispan but in due course.
I hope to find time to contribute back more, and in better ways, like
concrete enhancements or issues with test cases as happened with jboss
cache, but right now that's the best I could. By the way, I'm using
the nick mart1ns in the infinispan freenode irc channel, feel free to
ping me there.
Regards,
-- Eduardo
..............................................
http://emmartins.blogspot.com
http://redhat.com/solutions/telco
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache