Thoughts on 2.0 API

[genman]

Probably the boat has already left the dock, but here are some thoughts...

There's org.jboss.cache.factories and org.jboss.cache.pojo.factory, in addition to being inconsistent, I sort of don't like the "ies" look. I also don't like "factory" as a package in general, because I don't think it has enough weight (number of classes and scope of use) to warrant separation. Its use is integral to how a user builds a cache, so it should be immediately obvious where to get it.

I also doubt that having the Factory be an interface is better than an abstract base class. A simple "abstract Cache createCache(...)" that users can replace and a "static Cache createDefaultCache" would be far more useful.

import org.jboss.cache.Cache;
import org.jboss.cache.CacheFactory;
Cache c = CacheFactory.createDefaultCache();
c.put(Fqn.ROOT, "a", "b");
// versus
import org.jboss.cache.Cache;
import org.jboss.cache.factories.CacheFactory;
import org.jboss.cache.factories.DefaultCacheFactory;
CacheFactory cf = new DefaultCacheFactory();
Cache c = cf.createCache();
c.start();
c.put(Fqn.ROOT, "a", "b");


Having a start parameter, like "Factory { create(Config c, boolean start) }" is sort of hokey... In general, the 90% use case is to create a started cache, if you're going through Java. A "Configuration" could state if a cache should be in started state for the 10% of cases. Example:

Configuration c = ...
c.setStartOnCreate(false); // or something like that
Cache c = CacheFactory.createCache(c);
c.start();


CacheSPI () { } ... Having a List that can be changed seems better than two additional methods. A List is for adding and removing stuff, right?

CacheSPI spi = ...
spi.getInterceptorChain().add(new MyInterceptor());
// or
spi.getInterceptorChain().add(4, new MyInterceptor());
// or
spi.getInterceptorChain().addAll(4, List l);


I'm also thinking a JSR-77 sort of Statistics class would be in order. Having a grouped bunch means you can add new statistics without changing the CacheSPI interface itself.

Why does Region not have a "CacheListener"? There are 6 methods in CacheSPI for a listener, but none in Region.

I guess what'd be nice is a:

Fqn f;
CacheListener cl;
Region r = Cache.getRegion(f);
r.getCacheListeners().clear();
r.getCacheListeners().add(cl);
Cache.getRegion(Fqn.Root).clear();
// not sure how well defined this might be


I would probably move RegionImpl->Region.

Having interfaces sort of prevents you from extending your API (easily). Again, what's the benefit of an interface over a concrete class in this situation? Region sounds like something you'd want to extend in the future. One scenario is if you had different interceptor configs or cache loaders per region in the future.

The new API needs to justifiable, otherwise your users will simply do this:

TreeCache tc = ...
Cache c = tc.getCacheSPI();

and the hell to your factories ;-)

[ben.wang]

"genman" wrote:
Probably the boat has already left the dock, but here are some thoughts...

Certainly there is room for discussion and make some changes, if ncessary. That's what a devleoper release is for anyway. :-)

There's org.jboss.cache.factories and org.jboss.cache.pojo.factory, in addition to being inconsistent, I sort of don't like the "ies" look. I also don't like "factory" as a package in general, because I don't think it has enough weight (number of classes and scope of use) to warrant separation. Its use is integral to how a user builds a cache, so it should be immediately obvious where to get it.

Agreed. Actually, I don't use org.jboss.cache.pojo.factory package now. Use it will trigger a RuntimeException. The factory class is located directly under org.jobss.cache.pojo.

I also doubt that having the Factory be an interface is better than an abstract base class. A simple "abstract Cache createCache(...)" that users can replace and a "static Cache createDefaultCache" would be far more useful.

I don't quite agree that the current way of creating a default cache is that bad though. If you don't pass in the start flag then the default is to start the cache life cycle. Isn't this what you are asking for?

Why does Region not have a "CacheListener"? There are 6 methods in CacheSPI for a listener, but none in Region.

Sure this would be nice. Manik can say more definitely. But we do have a plan to elevate the Region into first class construct.

[genman]

I don't quite agree that the current way of creating a default cache is that bad though. If you don't pass in the start flag then the default is to start the cache life cycle. Isn't this what you are asking for?

I guess I wasn't paying attention. :-/

Sure this would be nice. Manik can say more definitely. But we do have a plan to elevate the Region into first class construct.

This would probably be my biggest suggestion. I'm not sure what the roadmap is, I did poke around JIRA and find these two issues:

http://jira.jboss.com/jira/browse/JBCACHE-64
http://jira.jboss.com/jira/browse/JBCACHE-86

Both of these suggest you'd want to extend Region in the future.

I also think that something like this:

Cache tokyo = Cache.getRegion("/tokyo").getCache();

would be really cool. Something like how List.subList is done. It'll allow you to segregate cache access better in your application.

A couple of other things:

putIfNull -> putIfAbsent

The difference between the key doesn't exist and exists but points to null is a subtle one. The JavaDoc seems to imply "putIfAbsent"...

 * Puts a key and a value into the current node's data map if nothing exists
under the key.
 * Does nothing if the key already exists.

you could simply "borrow" the javadoc from 1.5 ConcurrentMap:

 If the specified key is not already associated with a value, associate it with the given value. This is equivalent to

 if (!node.getKeys().contains(key))
 return node.put(key, value);

Actually, one thing I did notice is that "put" no longer returns the old value, was that intentional?