jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Dürig <mdue...@apache.org>
Subject Re: oak-spi: NodeStore, NodeStateBuilder and CommitHook
Date Wed, 16 May 2012 09:10:35 GMT

Hi,

On 16.5.12 9:40, Jukka Zitting wrote:
> 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.

Yes I took a look at these already. This looks very similar to my very 
first approach in the sandbox last year (the stuff was called NodeDelta 
back then).

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

All very well but... ;-)

>
> 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 don't think this will reasonably work. There is just not enough 
information in the raw states to reconstruct a JSON diff that also 
includes the move operations. In particular, the information which is 
missing is the order of the original operations.

After all this is the reason why I chose to track the transient changes 
separately. The problem with that approach (as we both noted) is where 
to but this additional "global" state.

Michael


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