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-API - some comments and suggestions [1]
Date Fri, 13 Apr 2012 15:20:35 GMT

On 13.4.12 13:29, Angela Schreiber wrote:
> here the summary of some initial observations that i made while
> looking at the oak-api today:

Thanks for the review. Before I comment on the individual points let me 
explain the overall motivation and the general idea that lead to the 
current design and the implementation trade off's, which may make things 
seem strange.

The idea is to use a branch/merge metaphor as brought up by Jukka [1]. 
That is, conceptually you do a branch to a private copy, make your 
modifications there and merge it back. This is reflected in 
NodeStore.branch() and NodeStore.merge(). The Connection interface 
specialises these details away such that users of that interface can 
'just edit' without having to keep track of the branch/merge stuff. This 
is most apparent in that NodeStore.merge() takes a target NodeState into 
which the changes are merged. Connection.commit() OTOH always uses the 
base node state (the one from which the branch was obtained) as target. 
See ConnectionImpl.commit().

The reason for having NodeStateEditor and TransienNodeState as separate 
classes is to separate modification of state from representation of 
state. A TransienNodeState represents the state of a sub-tree at a given 
point in time. It does not know/care how it got there and where it will 
ultimately go. A NodeStateEditor in contrast has the knowledge how to 
modify the state of a sub-tree and how to merge such modification back 
into the store.

Regarding TransientNodeState: as hinted above and as already mentioned 
in an earlier discussion [2], a TransienNodeState represents the state 
of a *whole* sub-tree.

[1] http://markmail.org/message/jbbut6vzvmmjqonr
[2] http://markmail.org/message/smwwgdpxfl67emgs

> a) NodeState is never used in oak-jcr
> as far as i saw the NodeState interface is never used in the
> scope of oak-jcr in contrast to TransientNodeState that
> is omnipresent.
> basically i got the impression that TransientNodeState
> isn't really about transient JCR modification but in fact
> *is* the representation of the persisted MK node state on
> the oak-API. oak-api consumers would never need to access
> the mk-NodeState.

*Is* the representation of persisted MK nodes + transient modifications 
would be more correct.

> Suggestion(s):
> - remove NodeState from oak-api
> - move to 'kernel' package or really use the corresponding
> interfaces from mk.model package

Might make sense...

> - still rename TransientNodeState to (variants following)
>  > NodeState
> in this case i would rename KernelNodeState to KernelNode
> as on mk level it is called Node again.
>  > NodeData, NodeInfo, Node* (to distinguish from jcr Node)
> in this case NodeState would still refer to mk and
> the MK-API should use the term NodeState instead of Node
>  > Apply the same pattern then to PropertyState

I'd go for NodeState instead of TransientNodeState and rename 
KernelNodeState to KernelNodeData and rename the current NodeState to 
NodeData. My reasoning: the current NodeState is immutable (and thus 
represent a pure value i.e. data). The word state implies mutability 
(after all OOP is about mutation of state all over) so renaming 
TransientNodeState to NodeState should make sense. Also the oak API will 
be more publicly visible than the lower level MK API. So having catchy 
names there is a plus.

> b) TransientState not extending from NodeState
> at a first glance i found that pretty confusing. with the comment
> made in a) that actually makes sense... but the name is confusing.

It confused my self initially. Another indicator that the naming is 
broken. NodeState is immutable. So there is no way for 
TransientNodeState to extend from it.

> in fact that TransientNodeState already looks much more like a
> JCR node than like a mk-NodeState as it refers to it's parent, has
> an editor (-> see below) etc.
> Suggestion(s):
> -> see above.
> c) NodeStateEditor : general
> the name implies that it is a TransientNodeStateModifier
> but in the implementation it is a mixture of ChangeLog|Set and
> a ItemModified (not only nodes)... that looks like a confusion
> of intents to me.

The change log is an implementation detail. Other implementations might 
rely on an diff/merge approach and don't need a change log at all. So 
I'd refrain from using ChangeLog or ChangeSet as part of the class name.

> Suggestion(s):
> - at least rename NodeStateEditor to something like Item*Modifier
> or ...Editor where * would match whatever solution we come up
> with in point a).

As I said above a TransientNodeState consists of a whole subtree. So 
NodeStateEditor seems not too bad.

> - if we made TransientNodeState the main node-like interface
> on the oak-api that is really distinct from the NodeState
> concept in mk and the core impl, i would like to suggest to
> move the (few) modifier methods to that interface.
> for the sake of an API that is easy to read and easy to use.
> - The editor was then not used to edit states but could be e.g.
> renamed to ChangeSet|Log (or similar). it's single
> purpose was then to collect the changes.

See my introduction for why this is so. I think it makes sense to keep 
these apart on the API level. But I'm in favour to implement something 
on top of these interface which looks like a node which can directly be 

> - I don't know yet to which extend a consumer of the oak-api
> really need to get in touch with that changeset. but it felt
> natural to me to pass the changeset to the Connection#commit.
> ... but i would need to fiddle around with the code to get a
> clear picture.
> d) NodeStateEditor#getBaseNodeState() NodeState
> this method is never used from a consumer of the oak-API.
> to me that is again (see also NodeState interface) an indication
> that we expose information that was in fact an implementation
> detail... it's only the oak-core implementation that needs to
> know that.
> Suggestion:
> - remove that method for the oak-api
> - use implementation specific method or somehow add mk.model package.


> e) NodeStore, NodeStateDiff, ChildNodeEntry:
> similar to NodeState i am not sure if that those really needed and
> properly located in oak-api. so far i didn't find any usage in
> oak-api.
> Suggestion:
> - remove those classes from the oak-api until the API-consumer
> (oak-jcr) would really use it or if we know for sure where
> it will be used very soon.
> - use mk.model interfaces instead in the oak-core implementations
> that are in the kernel package.

Ack for ChildNodeEntry and NodeStore to move to kernel package.
NodeStateDiff is currently not used at all. I think its intent is as a 
callback to be implemented for observation listeners. Jukka?

> f) PropertyState
> as stated at point a) this interface currently serves two
> purposes as it is used both in NodeState and TransientNodeState.
> that looks like a mistake to me in the light of the 2 node state
> interfaces not being connected by super-sub-interface relationship.

I don't see why such as interface relationship should be needed. 
PropertyState represents an immutable value (like String). String is 
also used all over the place without having sub/super-type relationships.

> Suggestion:
> - remove PropertyState as a notion of MK-property state from the
> oak-api
> - add a Property* interface where * would be according to the name
> of the oak-api naming convention for item/states being exposed
> by that api... right now the consistent name on oak-api probably
> was "TransientPropertyState" ;)

I don't see the need to introduce a another representation here. 
PropertyState serves its purpose which is to represent an immutable value.


> so, looking at the findings in this first set of comments, i have
> the impression that there are right now a lot of unused interfaces
> in oak-api... unless someone objects i would like to go ahead
> and clean up the oak-api package as this will help us getting
> a clearer picture of what we have and what is really missing.
> for the findings that need more fundamental changes (e.g.the
> editor/changeset discussion), i would like to get your feedback
> on whether that was feasible/desirable (in the first place from
> michael, that was working on this).
> kind regards
> angela

View raw message