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:52:21 GMT


On 13.4.12 16:34, Angela Schreiber wrote:
> hi michael
>
> thanks for your comments (and your patience :-).

:) and yours...

>
> i committed a first set of changes that didn't require
> too many fundamental changes in
> http://svn.apache.org/viewvc?rev=1325799&view=rev

Thanks looks good.

>
> hope you can continue here and include your ideas as
> listed below... that all seems reasonable to me.

Sure. I created OAK-65 for tracking the renaming stuff since this 
touches a broader scope. I will go ahead within OAK-18 for the other stuff.

>
> kind regards
> angela
>
>
>
> On 4/13/12 5:20 PM, Michael Dürig wrote:
>>
>>
>> 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
>> modified.
>>
>>>
>>> - 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.
>>>
>>
>> Ack.
>>
>>>
>>> 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.
>>
>> Michael
>>
>>
>>>
>>>
>>> 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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>

Mime
View raw message