>> 5. Cache needs move(), not Node. The reason is clear if you see >> NodeImpl. >> > Yes, this is one I have been toying with. What do others think? Using move() does seem natural, you're in a node and want to move it and his children to other fqn. The only downside I can see is people using it from the Cache interface and not realising that this is Node method which might leave them wondering which node you are moving...etc. I guess a move(Node1, Node2, newFqn) might be easier to read, but this is a method that could well live in Node as it does right now, leaving space for other things in the Cache interface.

Galder Zamarreño Sr. Software Maintenance Engineer JBoss, a division of Red Hat IT executives: Red Hat still #1 for value http://www.redhat.com/ promo/vendor/ -----Original Message----- From: jbosscache-dev-bounces(a)lists.jboss.org [mailto:jbosscache-dev- bounces(a)lists.jboss.org] On Behalf Of Manik Surtani Sent: 20 November 2006 16:43 To: genman(a)noderunner.net Cc: jbosscache-dev(a)lists.jboss.org Subject: Re: [jbosscache-dev] JBCACHE-867 On 20 Nov 2006, at 04:25, Elias Ross wrote: > > > I finished most of getting rid of the TreeCacheProxyImpl > references. It took > more time than anticipated. > > All said, it was about 1800 lines of adds and 2000 lines of removes. > TreeNode and DataNode are pretty empty. There's now a NodeSPI > which might > need some work, but it's pretty pleasant. I also refactored out a > lot of > lock stuff into something that hangs off of NodeSPI. > > Some tests may have been messed up (buddy replication and the move > feature > and probably some others) and I haven't had time to figure these > complicated > ones out. I hope this doesn't upset anybody. If somebody could > help fix > these, that would be great. No this is fine, this was on the cards for me but you've saved me a bit of work here. :-) Now just to make sure none of the functionality's broken. > > 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? > 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. > 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. :-) > 5. Cache needs move(), not Node. The reason is clear if you see > NodeImpl. > Yes, this is one I have been toying with. What do others think? > In the current design, CacheSPI has to have all the methods of > Cache. Why > couldn't CacheSPI be written as a separate object? (It could, but > then you > would just have to delegate ...) > > > > _____________________________________________________________________ > _ > ______________ > Sponsored Link > > Mortgage rates near 39yr lows. > $510k for $1,698/mo. Calculate new payment! > www.LowerMyBills.com/lre > _______________________________________________ > jbosscache-dev mailing list > jbosscache-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jbosscache-dev _______________________________________________ jbosscache-dev mailing list jbosscache-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/jbosscache-dev

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.

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

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