[manik]

org.jboss.cache.marshall.ObjectStreamFactory

public ObjectInputStream createObjectInputStream(InputStream in) throws IOException;

JavaObjectStreamFactory and JBossObjectStreamFactory would implement it.

VAM would add:

public Object objectFromInputStream(InputStream is) throws Exception

Yeah I'm ok with this.

[manik]

"genman" wrote:

Also, you probably noticed I made a bunch of additions to NodeSPI. These probably need some review before finalization, I would think.

Yes, why is there a getChildrenMap() on NodeSPI(), doesn't Node.getChildren() (returns a Set) suffise?

[manik]

If we're exposing the TM directly via the Cache interface, I don't need that. Having the cache stick a ref to its TM in Configuration.runtime would serve the need.

Don't we expose this via the runtime right now?

cache.getConfiguration().getRuntimeConfig().getTransactionManager();

If this is enough I'll move getTransactionManager() from Cache to CacheSPI.

[manik]

Another thing I've thought about is renaming the TreeCache class to CacheImpl or something. We don't code to be written against the TreeCache class itself. There's a lot of legacy code written that way; changing the class name forces a shift to the new API.

+1, this is a good idea.

A kinder gentler way is to create CacheImpl, and then make TreeCache a trivial subclass. Then mark TreeCache deprecated with a comment it will be removed in 2.1 or 2.2.

Do we really want this? Like you said we don't want people talking to the implementation directly at all, especially since we can't guarantee correct behaviour with the interceptor chains, etc. I'd rather remove this altogether, like you said forcing people to write to the new API.

[manik]

It is hard to say exactly what do I need from CacheSPI (or NodeSPI) now. When will you check in the new API or has it been updated in the Wiki?

I have thought people who wants to customize the Cache behavior should use CacheSPI. Or has it been changed?

Not really a case of customisation, more a case of being able to access cache internals.

What I meant was extensions and plugins would be able to use the CacheSPI.

[manik]

"genman" wrote:

I "proposed" the API to access the node SPI should be Node.getNodeSPI(). This was cleaner rather than having to make an iffy cast all the time. I believe that a Cache.getCacheSPI() would be for the best.

-1, see my comment on the post above. I want to protect internals from client code.

[brian.stansberry]

If we're exposing the TM directly via the Cache interface, I don't need that. Having the cache stick a ref to its TM in Configuration.runtime would serve the need.

Don't we expose this via the runtime right now?

Yeah, we do. I expressed that badly. Also I double checked -- if the cache uses the TMLookup to find the TM, it then adds the TM to the runtime. So it's always available that way.

So no reason to expose it via the Cache interface.

[brian.stansberry]

A kinder gentler way is to create CacheImpl, and then make TreeCache a trivial subclass. Then mark TreeCache deprecated with a comment it will be removed in 2.1 or 2.2.

Do we really want this? Like you said we don't want people talking to the implementation directly at all, especially since we can't guarantee correct behaviour with the interceptor chains, etc. I'd rather remove this altogether, like you said forcing people to write to the new API.

+1 from me on not doing that. I added the idea in case any 'kinder, gentler' readers were concerned about changing the name. I'm a heartless thug. ;-)

[manik]

I'm a heartless thug.

This is what we've come to expect from you, Brian. :-)

[vblagojevic]

+1 for renaming. I agree with a heartless thug.

[genman]

A few last minute comments:

1. putIfNull -> putIfAbsent, See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/ConcurrentMap.html . People might get confused with "null" versus absent.

2. Remove the print* methods in NodeSPI. Or at least document them. Consider creating a separate util class to print a node.

3. removeChildDirect(Fqn) and removeChildDirect(Object) is a little confusing, since Fqn is a subtype of Object. I would probably suggest just having one or the other.

4. Some of the NodeSPI data methods appear to be just convenience methods and aren't strictly necessary given that getDataDirect() returns an externally modifiable map. For instance, clearDataDirect() really just is the same as NodeSPI.getDataDirect().clear(). So, it would be nice to understand the reason for these extra methods.

[manik]

Good comments.

"genman" wrote:

1. putIfNull -> putIfAbsent, See http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/ConcurrentMap.html . People might get confused with "null" versus absent.

Agreed.

"genman" wrote:

2. Remove the print* methods in NodeSPI. Or at least document them. Consider creating a separate util class to print a node.

My issue with a separate util class is that it will mean maintaining yet another public interface/class.

+1 to documenting them better.

"genman" wrote:

3. removeChildDirect(Fqn) and removeChildDirect(Object) is a little confusing, since Fqn is a subtype of Object. I would probably suggest just having one or the other.

It does look confusing, but for the sake of convenience - and a lot of unnecessary Fqn creation - it makes sense having the 'Object' version. Helps avoid a lot of unnecessary stuff like

node.getChildDirect(new Fqn(childName))

which in turn simply does

if (fqn.size() == 1) return children.get(fqn.getLastElement());

Justifying the Fqn version, it cleans up unnecessary and repeated looping in interceptors and CacheImpl searching for a child node several nodes deep.

"genman" wrote:

4. Some of the NodeSPI data methods appear to be just convenience methods and aren't strictly necessary given that getDataDirect() returns an externally modifiable map. For instance, clearDataDirect() really just is the same as NodeSPI.getDataDirect().clear(). So, it would be nice to understand the reason for these extra methods.

This is a good point. Will revisit thise. Their Node interface counterparts that went up the interceptor stack made sense, but the 'direct' versions do not.

[manik]

"genman" wrote:

4. Some of the NodeSPI data methods appear to be just convenience methods and aren't strictly necessary given that getDataDirect() returns an externally modifiable map. For instance, clearDataDirect() really just is the same as NodeSPI.getDataDirect().clear(). So, it would be nice to understand the reason for these extra methods.

This is a good point. Will revisit thise. Their Node interface counterparts that went up the interceptor stack made sense, but the 'direct' versions do not.

Actually, there is a good reason to still maintain these methods. At some point I plan to do away with synchronizing these methods, and have my own lock checks to ensure the caller has appropriate locks. And to do this, I need to make sure the Map returned to getDataDirect() is an unmodifiableMap.

In fact, all read calls would return unmodifiableMap/Sets, and the only way to write in to them would be to use the write methods.

Read and Write methods would then impose a check on the caller to see if appropriate locks are available, e.g.,

pubic Map<Object, Object> getDataDirect()
{
 if (!getLock().getReaderOwners().contains(Thread.currentThread()))
 throw new LockingException();

 // defensive copy + unmodifiable
 return Collections.unmodifiableMap(new HashMap(data));
}