[jbosscache-dev] JBCACHE-867

[Elias Ross]

Hi guys.

What I tried initially was to have Node call methods on Cache.  The problem I
ran into was that some methods on TreeCache didn't have a method part of the
Cache interface.  Actually, having Node call methods on Cache doesn't make
much sense, as Node might need to call through the interceptors something
Cache does not provide.

Either:

1.  Node delegates to TreeCache.  (Rather than through Cache.)  This I don't
like since I don't want to tie Node (even internally) to a specific
implementation.
2.  A new interface is created for TreeCache, and Node delegates through
that.  This is probably okay.
3.  Instead (or in addition) some sort of AOP trick is done to a Node,
similar to what was done with TreeCacheProxyImpl, which is sort of putting
things back to the old way.

Actually, 3 is probably better (but I'm not an AOP expert).  At the very
least, I'd like to see something to automate this sort of pattern:

if (cache....isBypassInterceptorChain()) 
  // modify Node
} else {
  // call method on cache
  cache.doSomething()
}

(I think AOP might have a performance penalty, I don't know.)

Anyway, I hope you all accept that whatever changes I might make, they're
entirely experimental and demonstrative...

Going back a week ...

> >> I would like to suggest the following things:
> >> 1.  Cache should not implement Node, it's confusing when reading an
> >> implementation of Cache
> >
> > We toyed with this quite a bit and in the end we decided to go ahead
> > with it since it lets people use the Cache directly rather than to
> > get a hold of a Node first.  In what ways do you feel this is  
> > confusing?

Well, one object has methods related to the Cache, some related to Node.  But
when you use a Cache, how do you tell (as a user) which method operates on
which?  It's not clear when writing your code.

I think having put() in Cache will encourage users to store all their data in
the single root Node.  A single node cache doesn't work well with cache
loaders and eviction policies.  Users need to have their data in separate
nodes anyway.

And some methods are confusing, or don't make much sense (or are pointless)
to call on a cache.

Cache cache = ...;
cache.clearData(); // only clears the root, not the whole cache
cache.getParent(); // pointless
cache.move(); // will never work
cache.getFqn(); // pointless

> >
> >> 2.  Node sould include useful methods of TreeNode, such as getChild
> >> (Object)
> >> and getName()
> >
> > There is getChild(Fqn) - and I want to standardise on this.  The Fqn
> > passed in is relative to the Node in question so you can have access
> > to direct children.
> >
> > Similarly with getName(), I prefer getFqn().getLast() simply because
> > it cements the use of Fqns when it comes to identifying Nodes, and I
> > don't think adding a boatload of convenience methods helps the
> > interface very much.

Eh, I don't feel too strongly about this.  I do think having both
getChild(Fqn) and getChild(Object) would be confusing, so I concede you're
probably right.  It just seems that if you have a lot of (test) code with
getChild(x).getChild(y) there must have been a good reason to have the API to
begin with.

> >> 3.  Cache should have a getCacheSPI()
> >
> > Again, -1.  I had this in place initially (again as a temp measure)
> > and refactored this out.  The purpose of the SPI is to give users
> > access to an 'advanced' API which would only be of value to people
> > implementing Cache Loaders, Cache Listeners or custom Interceptors.
> > Not for typical use of the cache.  And as such, this interface should
> > only be injected into such Cache Loaders, Listeners and Interceptors.
> >
> >> 4.  CacheSPI should not extend Cache
> >
> > In the case of Cache Loaders, Listeners and Interceptors, they would
> > need access to both interfaces.  Don't want to pass in a Cache *and*
> > a CacheSPI, and don't want Cache.getCacheSPI() for the reasons
> > outlined above.  :-)

I think giving your users the option to implement Cache and CacheSPI as
separate objects might be a good thing.  Or, mix and match a Cache with a
different SPI.  (Maybe a stretch...)

If you ever consider adding security, it would make sense to "hide" the SPI
from users.  I guess you could always add per-method checks or provide some
sort of wrapper, similar to Collections.unmodifiableMap().

One does have to ask, why isn't it trivial to have the TreeCache implement
the CacheSPI interface?  There's a lot of methods and it's not fun to create
all of those...

And if I could add to my criticism, I would suggest that Region needs to
return some sub-interfaces.  Region.activate() does not suggest to me
anything about replication.  markNodeCurrentlyInUse() does not indicate
eviction behavior.  I think it would be good to group methods like so:

Region r;
r.replication().activate();
r.eviction().setPolicy( ... );



 
____________________________________________________________________________________
Want to start your own business?
Learn how on Yahoo! Small Business.
http://smallbusiness.yahoo.com/r-index