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
