[jira] [Comment Edited] (OAK-6425) Make the CompositeNodeStore thread-safe

["Chetan Mehrotra (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-6425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16075993#comment-16075993 ] 

Chetan Mehrotra edited comment on OAK-6425 at 7/6/17 6:24 AM:
--------------------------------------------------------------

bq. when the merge() method is in progress and the changes has been already committed to some node stores, but not to the others. If someone calls getRoot(), they will get a state in which only part of the merge changes are available.

This should not be the case if only one of the mount is writeable. Supporting multiple writeable mounts is anyway not possible as then it would not be possibly to ensure session save is atomic across mounts

bq. in the same merge() method first we're rebasing the changes, applying the commit hooks and then merge them with node stores. What happens if someone else have their changes committed in between applying the commit hooks and the merging part?

This part is tricky with current impl. From my understanding
* SegmentNodeStore - Serializes the commits so CommitHook are run within lock. So its not possible to have merge conflicts due to changes done by CommitHook (say in property indexes). Merge conflict in normal content would anyway fail the merge
* DocumentNodeStore - Here the commit hooks can run concurrently. In case of conflict it does a auto retry and tries to merges again for some attempts. This is done to ensure that conflict introduced by CommitHooks can get resolved as they are run again. 

Now here we run CommitHook before NodeStore passing an EmptyHook. So not sure whats the right way

[~mreutegg] [~mduerig] Thoughts? Have a look at CompositeNodeStore#merge for current behaviour (prior to lock patch). 

Update - Thinking more I think current implementation should be changed and just pass on all hooks and not do any rebasing/processing here




was (Author: chetanm):
bq. when the merge() method is in progress and the changes has been already committed to some node stores, but not to the others. If someone calls getRoot(), they will get a state in which only part of the merge changes are available.

This should not be the case if only one of the mount is writeable. Supporting multiple writeable mounts is anyway not possible as then it would not be possibly to ensure session save is atomic across mounts

bq. in the same merge() method first we're rebasing the changes, applying the commit hooks and then merge them with node stores. What happens if someone else have their changes committed in between applying the commit hooks and the merging part?

This part is tricky with current impl. From my understanding
* SegmentNodeStore - Serializes the commits so CommitHook are run within lock. So its not possible to have merge conflicts due to changes done by CommitHook (say in property indexes). Merge conflict in normal content would anyway fail the merge
* DocumentNodeStore - Here the commit hooks can run concurrently. In case of conflict it does a auto retry and tries to merges again for some attempts. This is done to ensure that conflict introduced by CommitHooks can get resolved as they are run again. 

Now here we run CommitHook before NodeStore passing an EmptyHook. So not sure whats the right way

[~mreutegg] [~mduerig] Thoughts? Have a look at CompositeNodeStore#merge for current behaviour (prior to lock patch)



> Make the CompositeNodeStore thread-safe
> ---------------------------------------
>
>                 Key: OAK-6425
>                 URL: https://issues.apache.org/jira/browse/OAK-6425
>             Project: Jackrabbit Oak
>          Issue Type: Improvement
>          Components: composite
>            Reporter: Tomek Rękawek
>            Assignee: Tomek Rękawek
>             Fix For: 1.8, 1.7.4
>
>
> The CompositeNodeStore, unlike other *NodeStore implementations, is not thread safe (eg. two concurrent merge() invocations may leave the repository in inconsistent state).



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)