[jbosscache-dev] JBCACHE-867

[Manik Surtani]

Elias,

On 28 Nov 2006, at 06:28, Elias Ross wrote:

>
> 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.

I don't get this bit - Node does not call methods on Cache.  Keep in  
mind that phase #1 of the refactoring (a.k.a. JBoss Cache 2.0.0) is  
just to provide a new interface over an existing implementation, with  
whatever glue (TreeCacheProxyImpl) that may be necessary.

A proper re-architecture of the implementation to take advantage of  
the new interfaces, possibly using AOP as you mentioned, is not due  
till 2.1.0.  I really don't want to change everything in one go for a  
number of reasons, the biggest being the time needed to stabilise  
such a release.

Anyway, see my comments below:

>
> 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.

For this release, I don't have a problem with this and I think this  
is the best approach.

> 2.  A new interface is created for TreeCache, and Node delegates  
> through
> that.  This is probably okay.

-1.  Another interface for an impl class that may disappear in  
2.1.0?  No thanks... :-)

> 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.

Don't want to bring in AOP in 2.0.0.  Leave that as a possible route  
for 2.1.0.

>
> 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...

Just make sure these experimentative changes don't get checked in to  
HEAD by mistake, as I do need to release another alpha off HEAD once  
I fix a few outstanding issues.

>
> 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.

It doesn't matter.  A Cache is a Node - the ROOT Node.  So when you  
use operations like put(), get(), etc., you know you are doing these  
on the root node.  On the other hand, cache-wide operations like  
getConfiguration() and addListener(), etc. apply across the cache  
since these are not specific to a Node.

>
> 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.

This is a good point, as users not familiar with the tree struct  
would not know that they need to getChild() first and then do a put().

>
> 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

As these are inherited from Node, yes.  Again, if you look at Cache  
as the root Node, then some of these begin to make more sense.

Ok, lets look at an alternative - regardless of the implementations  
behind the interface as these can remain the same, even - Cache does  
NOT implement Node, but exposes a getRootNode() helper method.  This  
would force users to get a hold of the Node before storing any data  
(unless they use the backward-compatible put(Fqn, Key, Value) methods  
on Cache), forcing an awareness of the tree structure and the need to  
organise your data in a tree.

Hmm.  I'm not completely against it, what do others think though?


>
>>>
>>>> 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.

You don't *need* to do that - you could do getChild(new Fqn("/x/y"));

>
>>> <SNIP />

>>>> 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...)

I don't get this.  Users will not implement Cache and CacheSPI.  We  
will.  And pass these impls to user-implementations of Cache Loaders/ 
Listeners/Interceptors.

>
> 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().
>

And this is fine, something we can add even in the current state.   
But remember that the implementations in 2.0.0 are just wrappers  
around existing code.  The real re-implementation of what is under  
the covers is for 2.1.0.

> 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( ... );


Yes, this region interface needs a bit of rethinking - it was  
initially just a hash together of eviction.Region and marshall.Region  
and some of these concepts don't make too much sense to one another.   
There is a separate thread on this, I'd suggest you post there.


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