You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by Robert Munteanu <ro...@apache.org> on 2016/09/09 15:09:50 UTC
[discuss] Stop wrapping unknown NodeState instances with a
MemoryNodeStore?
Hi,
I'd like branch my 'Are all NodeBuilders required to inherit from the
MemoryNodeBuilder?' thread to put the focus back on the root issue.
Some repository/workspace initializers unconditionally wrap a passed
NodeState with a MemoryNodeStore, given that up till now all NodeState
instances extend the MemoryNodeState. [1][2][3]
However, once we get a NodeState that does not inherit from a
MemoryNodeState it all breaks down - like in my multiplexing POC.
I currently have a hack^H^H^H^H isolated way of exposing a
MemoryNodeBuilder from a MultiplexingNodeBuilder, but I'd like a more
elegant approach.
I wonder if the issue is that the initializers don't get access to the
'original' NodeStore and we can extend the API to make it available? Or
maybe there are other ways of addressing it that I don't see due to my
limited exposes to Oak's core.
Thanks,
Robert
[1]:�https://github.com/apache/jackrabbit-oak/blob/1fdae3a77e4172cf5716
6ffe77eb35a4bd93c76b/oak-
core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/write/Ini
tialContent.java#L118-L119
[2]:�https://github.com/apache/jackrabbit-oak/blob/1fdae3a77e4172cf5716
6ffe77eb35a4bd93c76b/oak-
core/src/main/java/org/apache/jackrabbit/oak/security/user/UserInitiali
zer.java#L94-L95
[3]:�https://github.com/apache/jackrabbit-oak/blob/1fdae3a77e4172cf5716
6ffe77eb35a4bd93c76b/oak-
core/src/main/java/org/apache/jackrabbit/oak/security/privilege/Privile
geInitializer.java#L58-L59
Re: [discuss] Stop wrapping unknown NodeState instances with a
MemoryNodeStore?
Posted by Tomek Rekawek <re...@adobe.com>.
Hi,
> On 23 Sep 2016, at 00:32, Michael Dürig <md...@apache.org> wrote:
>
> To this respect I think your fix is basically correct, it should just be applied deeper down. Instead of wrapping the base states before passing them to the MemoryNodeStore constructor, I think that constructor should do the wrapping in case the passed base state is of a different type.
Thanks! I implemented this in the OAK-4847.
Regards,
Tomek
--
Tomek Rękawek | Adobe Research | www.adobe.com
rekawek@adobe.com
Re: [discuss] Stop wrapping unknown NodeState instances with a
MemoryNodeStore?
Posted by Michael Dürig <md...@apache.org>.
Hi,
I think the core of the problem is that the memory node store doesn't
always behave properly when initialised with something else than a
MemoryNodeState. Consider:
NodeState base = new AlienNodeState();
NodeStore store = new MemoryNodeStore(base);
NodeBuilder builder = store.getRoot().builder();
store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);
The merge call will throw an IAE if base.builder() does return a builder
instance that doesn't inherit from MemoryNodeBuilder.
To this respect I think your fix is basically correct, it should just be
applied deeper down. Instead of wrapping the base states before passing
them to the MemoryNodeStore constructor, I think that constructor should
do the wrapping in case the passed base state is of a different type.
Michael
On 22.9.16 2:49 , Tomek Rekawek wrote:
> Hi,
>
> I\u2019ve looked into this issue. I think it\u2019s caused by the fact that the squeeze() method sometimes doesn\u2019t wrap the passed node state with MemoryNodeStates, but return it as-is. I tried to wrap the state unconditionally in the initializers and it fixed the issue.
>
> Michael, Robert - do you think [1] is an acceptable solution? If so, I\u2019ll create a proper JIRA and merge the code.
>
> Regards,
> Tomek
>
> [1] https://github.com/trekawek/jackrabbit-oak/commit/cf3d73
>
Re: [discuss] Stop wrapping unknown NodeState instances with a
MemoryNodeStore?
Posted by Tomek Rekawek <re...@adobe.com>.
Hi Robert,
Thanks for the feedback.
> On 22 Sep 2016, at 15:13, Robert Munteanu <ro...@apache.org> wrote:
>
> Only thing I'm wondering is whether there is a scenario where
> performance would be greatly impacted since the NodeState contains lots
> of entries _and_ it's not a MemoryNodeState, so the newly added wrap
> method would basically copy everything.
The method is a shallow copy, it only copies the children references. Also, it’s being used in cases when we more or less know the state of the node state - it should be empty-ish, as it’s the root node during the repository initialisation phase.
Regards,
Tomek
--
Tomek Rękawek | Adobe Research | www.adobe.com
rekawek@adobe.com
Re: [discuss] Stop wrapping unknown NodeState instances with a
MemoryNodeStore?
Posted by Robert Munteanu <ro...@apache.org>.
On Thu, 2016-09-22 at 12:49 +0000, Tomek Rekawek wrote:
> Hi,
>
> I\u2019ve looked into this issue. I think it\u2019s caused by the fact that the
> squeeze() method sometimes doesn\u2019t wrap the passed node state with
> MemoryNodeStates, but return it as-is. I tried to wrap the state
> unconditionally in the initializers and it fixed the issue.
>
> Michael, Robert - do you think [1] is an acceptable solution? If so,
> I\u2019ll create a proper JIRA and merge the code.
>
> Regards,
> Tomek
>
> [1] https://github.com/trekawek/jackrabbit-oak/commit/cf3d73
>
That looks correct to me, although I may be missing some finer points.
Only thing I'm wondering is whether there is a scenario where
performance would be greatly impacted since the NodeState contains lots
of entries _and_ it's not a MemoryNodeState, so the newly added wrap
method would basically copy everything.
Upgrades maybe?
Robert
Re: [discuss] Stop wrapping unknown NodeState instances with a
MemoryNodeStore?
Posted by Tomek Rekawek <re...@adobe.com>.
Hi,
I’ve looked into this issue. I think it’s caused by the fact that the squeeze() method sometimes doesn’t wrap the passed node state with MemoryNodeStates, but return it as-is. I tried to wrap the state unconditionally in the initializers and it fixed the issue.
Michael, Robert - do you think [1] is an acceptable solution? If so, I’ll create a proper JIRA and merge the code.
Regards,
Tomek
[1] https://github.com/trekawek/jackrabbit-oak/commit/cf3d73
--
Tomek Rękawek | Adobe Research | www.adobe.com
rekawek@adobe.com
> On 9 Sep 2016, at 17:09, Robert Munteanu <ro...@apache.org> wrote:
>
> Hi,
>
> I'd like branch my 'Are all NodeBuilders required to inherit from the
> MemoryNodeBuilder?' thread to put the focus back on the root issue.
>
> Some repository/workspace initializers unconditionally wrap a passed
> NodeState with a MemoryNodeStore, given that up till now all NodeState
> instances extend the MemoryNodeState. [1][2][3]
>
> However, once we get a NodeState that does not inherit from a
> MemoryNodeState it all breaks down - like in my multiplexing POC.
>
> I currently have a hack^H^H^H^H isolated way of exposing a
> MemoryNodeBuilder from a MultiplexingNodeBuilder, but I'd like a more
> elegant approach.
>
> I wonder if the issue is that the initializers don't get access to the
> 'original' NodeStore and we can extend the API to make it available? Or
> maybe there are other ways of addressing it that I don't see due to my
> limited exposes to Oak's core.
>
> Thanks,
>
> Robert
>
> [1]: https://github.com/apache/jackrabbit-oak/blob/1fdae3a77e4172cf5716
> 6ffe77eb35a4bd93c76b/oak-
> core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/write/Ini
> tialContent.java#L118-L119
> [2]: https://github.com/apache/jackrabbit-oak/blob/1fdae3a77e4172cf5716
> 6ffe77eb35a4bd93c76b/oak-
> core/src/main/java/org/apache/jackrabbit/oak/security/user/UserInitiali
> zer.java#L94-L95
> [3]: https://github.com/apache/jackrabbit-oak/blob/1fdae3a77e4172cf5716
> 6ffe77eb35a4bd93c76b/oak-
> core/src/main/java/org/apache/jackrabbit/oak/security/privilege/Privile
> geInitializer.java#L58-L59