Mailing-List: contact oak-dev-help@jackrabbit.apache.org; run by ezmlm
Precedence: bulk
Reply-To: oak-dev@jackrabbit.apache.org
Received-SPF: pass (nike.apache.org: domain of anchela@adobe.com designates
 64.18.1.29 as permitted sender)
Message-ID: <4F88476A.1080709@adobe.com>
Date: Fri, 13 Apr 2012 17:34:02 +0200
From: Angela Schreiber <anchela@adobe.com>
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
 rv:1.9.2.18) Gecko/20110616 Thunderbird/3.1.11
MIME-Version: 1.0
To: <oak-dev@jackrabbit.apache.org>
Subject: Re: Oak-API - some comments and suggestions [1]
References: <4F881C12.10205@adobe.com> <4F884443.9080307@apache.org>
In-Reply-To: <4F884443.9080307@apache.org>
Content-Type: text/plain; charset="ISO-8859-1"; format=flowed
Content-Transfer-Encoding: 8bit

hi michael

thanks for your comments (and your patience :-).

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

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

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