1 2 Previous Next 27 Replies Latest reply on Jan 9, 2007 8:18 PM by manik

Last chance for any changes to the 2.0.0 API

manik Dec 15, 2006 9:00 AM

Guys,

I hope to cut an ALPHA2 of JBC 2.0.0 soon. After this, I want to freeze the API so there are no more changes to it during beta/cr releases, so let's get this right.

The original API I proposed is on this wiki page.

Since then, due to lessons learned implementing the interfaces plus a lot of feedback from the community (esp. genman) the interfaces shipped with 2.0.0.ALPHA1 was sufficiently different. And now, even more so since shipping ALPHA1.

Most of the public API interfaces (CacheLoaders, CacheListeners, Regions) are ok, but some (Cache/CacheSPI/Node/NodeSPI) still needs work. Currently, these contain some changes from genman, but still pretty incomplete IMO.

Here is what the Cache and Node interfaces and their implementations looks like now:

Some notes:

Node and Cache
These are public API which will be used directly by client code (including HTTP session repl code)

NodeSPI and CacheSPI
These are plugin developers APIs, and are injected into CacheLoader and CacheListener implementations. They provide access to a richer set of functionailty, including access to RpcManagers, TransactionManagers, etc.

Now here are the bits I have gripes with.

TreeCacheProxyImpl
Originally created as a delegate to link the new interfaces with TreeCache. I agree with earlier comments about this being unnecessary and TreeCache should just directly implement the new interfaces.

DataNode/TreeNode/AbstractNode/WorkspaceNode malarky
This lot needs revisiting, a lot of it is legacy that has been retrofitted into the new interface structure.

As such, here is what I propose we do with the new structure, a great simplification:

Node and Cache, and their SPIs
These interfaces remain the same, offering the same functionality and public API. Cache no longer extends Node, purely for the sake of readability and reducing confusion. Cache will contain helper methods such as put(Fqn, Key, Value) but this will delegate to the root node.

TreeCacheProxyImpl
Is dropped, TreeCache directly implements CacheSPI

DataNode and TreeNode
are dropped, and AbstractNode directly implements Node. Basic functionality is implemented in this abstract class.

NodeSPI and WorkspaceNode
Since these offer different degrees of specialisation based on whether a node resides in a transaction workspace or not, there are 2 separate interfaces. Crucially, NodeSPI offers both versioning and locking information.

UnversionedNode
This is simply what used to be called NodeImpl, renamed to be a little more descriptive. Fulfils the contract of NodeSPI and is the object that is stored in memory when using pessimistic locking. Would throw UnsupportedOperationExceptions when versioning methods are invoked. Extends AbstractNode for base functionality.

VersionedNode
This is simply what used to be called OptimisticTreeNode, again renamed to be a little more descriptive and to break the implied fact that it is only used by optimistic locking. (At the moment this is the case but the only thing that differentiates it from other nodes is that it is versioned). Extends AbstractNode and UnversionedNode.

Please let me know what you think, this should result in a much more maintainable and readable structure.

Oh, and sorry about the long post! :-)

Cheers,

1. Re: Last chance for any changes to the 2.0.0 API

genman Dec 15, 2006 2:56 PM (in response to manik)

Since I spent some time refactoring these classes, I have a few comments.

Actually, it's pretty close to what I'd like to see. "UnversionedNode" seems a little long for a name. And usually, you use naming to describe what something is, not what it isn't.

I still don't know how Node will proxy calls to Cache. Currently, NodeImpl is tied directly to TreeCache. How are these two classes going to interact?

Also, I'd like to hear some feedback on this issue: http://jira.jboss.com/jira/browse/JBCACHE-890 . I think we may want to add a couple of flags (three already come to mind.) Multiple booleans might be sufficient for the moment. Still, having a way to extend things cleanly might be nice in 2.0.
Actions
2. Re: Last chance for any changes to the 2.0.0 API

genman Dec 15, 2006 2:58 PM (in response to manik)

Also, you probably noticed I made a bunch of additions to NodeSPI. These probably need some review before finalization, I would think.
Actions
3. Re: Last chance for any changes to the 2.0.0 API

vblagojevic Dec 17, 2006 1:55 PM (in response to manik)

Manik,

This is an excellent proposal. Especially, branching off Cache from Node which was totally confusing! I would suggest to consider moving some methods from Cache to CacheSPI, like TransactionalManager and InvocationContext if possible. If you read carefully your post you already have this planned I believe.

Cheers.
Actions
4. Re: Last chance for any changes to the 2.0.0 API

manik Dec 18, 2006 6:36 AM (in response to manik)

Actually, it's pretty close to what I'd like to see. "UnversionedNode" seems a little long for a name. And usually, you use naming to describe what something is, not what it isn't.

Since access to nodes should take place using the interfaces (Node or NodeSPI), versioning and locking would exist in NodeSPI. As such, an implementation used for Pessimistic Locking, that does not implement versioning would have to throw UnsupportedOperationExceptions for get/setDataVersion() defined in NodeSPI.

And hence the descriptive implementation name: UnversionedNode - "a Node implementation that does not implement versioning as defined by the NodeSPI contract".

I agree that this negative implementation is a bit awkward, but unless we have a sub-interface to NodeSPI that defines versioning (and is even more awkward IMO) this is the cleaner of the two.
Actions
5. Re: Last chance for any changes to the 2.0.0 API

manik Dec 18, 2006 6:42 AM (in response to manik)

I still don't know how Node will proxy calls to Cache. Currently, NodeImpl is tied directly to TreeCache. How are these two classes going to interact?

At the moment I propose each node implementation has a reference to TreeCache since all "real" work happens by making calls to the TreeCache which then passes the call up the interceptor stack.

In 2.1.0, this can change, to clean up this otherwise spidery model, but for now I don't want to change this.
Actions
6. Re: Last chance for any changes to the 2.0.0 API

manik Dec 18, 2006 6:43 AM (in response to manik)

Also, I'd like to hear some feedback on this issue: http://jira.jboss.com/jira/browse/JBCACHE-890 . I think we may want to add a couple of flags (three already come to mind.) Multiple booleans might be sufficient for the moment. Still, having a way to extend things cleanly might be nice in 2.0.

Start a separate thread on this. I think that having such dynamically set series of flags on a node is a bit weird, wonder if we're better off with a limited set of getters/setters.
Actions
7. Re: Last chance for any changes to the 2.0.0 API

manik Dec 18, 2006 6:45 AM (in response to manik)

This is an excellent proposal. Especially, branching off Cache from Node which was totally confusing! I would suggest to consider moving some methods from Cache to CacheSPI, like TransactionalManager and InvocationContext if possible. If you read carefully your post you already have this planned I believe.

Some things, yes, but not all. Remember that typical end-user apps will not have access to the CacheSPI, just the Cache. Which includes things like Brian's http session repl code and Hibernate's TreeCacheProvider. Both of these would need access to the InvocationContext to set specific options.
Actions
8. Re: Last chance for any changes to the 2.0.0 API

ben.wang Dec 18, 2006 7:03 AM (in response to manik)

Manik, can you explain the difference between NodeSPI and WorkspaceNode? Why are they in parallel?

From my perspective, PojoCache needs to access NodeSPI. But what about WorkspaceNode?

Another question. if I stick with using the operation from Cache interface only, will there be any implication for performance reason for the new proposed change?

Finally, from this hierarchy, I still don't see a clean way to access CacheSPI (or NodeSPI), except to do:

CacheSPI spi = (CacheSPI)cache;

But this may subject to break in the future. Should we simply add a factory method for the SPI instance?
Actions
9. Re: Last chance for any changes to the 2.0.0 API

manik Dec 18, 2006 7:46 AM (in response to manik)

WorkspaceNodes are used by the optimistic locking interceptors so changes are not made to the nodes in the cache directly, but rather to the workspace nodes stored in the transaction workspace.

NodeSPI has access to locking mechanisms that don't exist in WorkspaceNodes (since each tx will have its own copy of a workspace node, there is no concurrency here)

There is no "access" to an SPI interface, as such. the SPIs are passed in to CacheLoader and CacheListener interfaces.

Is there anything specific PojoCache needs from the SPI interfaces?
Actions
10. Re: Last chance for any changes to the 2.0.0 API

brian.stansberry Dec 18, 2006 10:35 AM (in response to manik)

Re: exposing the TransactionManager, the http session repl code needs this. For http session repl, we use "BatchModeTransactionManager", not that main TM running in the AS. The external code to gets this from the cache, since it's the cache that creates the BatchModeTransactionManager.

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.
Actions
11. Re: Last chance for any changes to the 2.0.0 API

brian.stansberry Dec 18, 2006 10:40 AM (in response to 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.

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.
Actions
12. Re: Last chance for any changes to the 2.0.0 API

ben.wang Dec 18, 2006 11:05 AM (in response to 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?
Actions
13. Re: Last chance for any changes to the 2.0.0 API

genman Dec 18, 2006 2:40 PM (in response to manik)

"manik.surtani@jboss.com" wrote:

Also, I'd like to hear some feedback on this issue: http://jira.jboss.com/jira/browse/JBCACHE-890 . I think we may want to add a couple of flags (three already come to mind.) Multiple booleans might be sufficient for the moment. Still, having a way to extend things cleanly might be nice in 2.0.

Start a separate thread on this. I think that having such dynamically set series of flags on a node is a bit weird, wonder if we're better off with a limited set of getters/setters.

I think a get/set(boolean) for the NodeSPI is fine. I'm more concerned about the underlying implementation: It's just likely that the set of properties of a node will expand over time. What will inevitably happen is you'll want to add more methods to NodeSPI. The idea is to add some potential for future expansion.

If you think of the Tree as somewhat modeled off of a file system, the need for meta-data comes up time and time again. Maybe Java annotations can serve this role? But really you need something more dynamic.

Finally, from this hierarchy, I still don't see a clean way to access CacheSPI (or NodeSPI), except to do:

CacheSPI spi = (CacheSPI)cache;

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.
Actions
14. Re: Last chance for any changes to the 2.0.0 API

galder.zamarreno Dec 19, 2006 10:28 AM (in response to manik)

Guys, the change I'm currently working on to add VAM to JDBCCacheLoader would need an interface change. I haven't yet finished testing it to make sure that it's certainly more performant.

The change is this:

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

Finally, JDBCCacheLoader would call:

Object marshalledNode = getMarshaller().objectFromInputStream(is);

passing, InputStream is = rs.getBinaryStream(1); // ResultSet

Would you guys be happy for me to include this before finishing with http://jira.jboss.com/jira/browse/JBCACHE-879?

Without this methods, I would have to loop through the InputStream to create a byte[] containing this data which would a waste of time.
Actions

1 2 Previous Next

Go to original post