jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jukka Zitting <jukka.zitt...@gmail.com>
Subject Re: oak-spi: NodeStore, NodeStateBuilder and CommitHook
Date Wed, 16 May 2012 08:40:01 GMT
Hi,

On Mon, May 14, 2012 at 6:43 PM, Michael Dürig <mduerig@apache.org> wrote:
> We had similar discussions before [1] and it keeps popping up all over
> again: The NodeStore interface and friends all operate on NodeState and as I
> said before, I don't think there is a reasonable way to implement this.

See my last two commits to .oak.plugins.memory for a rough in-memory
outline of how I envisioned the NodeStore mechanism working.

> Having NodeState all the places is too general. Have a look at my commits
> related to OAK-100. The way things are implemented there is by lying:
> NodeStore.getBuilder() and NodeStore.setRoot() only accept very specific
> instances of NodeStates as their arguments and throw exceptions on all other
> instances.

I think we've been heading in a bit different directions with this
part of the codebase. A good time to sync up... :-)

The way I see it, the NodeStateBuilder (NSB) is a pretty simple
construct. All it ever needs to keep track is the set of changes to be
applied to a single base NodeState. The context, path and parent
information in the current KernelNodeState is in my view not needed.

And even further, the implementation of NSB.getNodeState() by calling
context.getNodeState(getPath()) goes against the (in my view useful)
concept of separate NSB instances being completely independent, each
being responsible only for tracking the changes made against that
specific instance.

I see where you're going by making the NSB context keep track of the
full set of transient changes. I had considered that to rather be a
concern of the TreeImpl class. Instead of a shared root builder, each
TreeImpl would keep a separate NodeStateBuilder instance for tracking
the set of transient changes that apply to just that TreeImpl
instance.

And I'd use NodeStore.compare() for constructing the JSON diff to be
passed down to the MicroKernel. Since the NodeStore implementation
knows the implementation class of most of the involved NodeState
instances, it can even construct things like move and copy
instructions in the NodeStateDiff handler.

> I think this is wrong and we should use more exact types for
> NodeStore (and related) interfaces instead of throwing exceptions at
> runtime.

We can look at reorganizing the method calls so that fewer class casts
are needed. But if the contracts are revised as described above I
don't think there'll be too many places where casting to
implementation classes is needed, and most of them will have
reasonable (though potentially much slower) fallbacks to generic
types.

BR,

Jukka Zitting

Mime
View raw message