jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vikas Saurabh (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (OAK-7710) CompositeNodeStore does not dispatch external events to observers
Date Wed, 22 Aug 2018 23:55:00 GMT

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

Vikas Saurabh commented on OAK-7710:
------------------------------------

Attaching somewhat fixed patch at  [^OAK-7710.patch]. It implements which we didn't really
require:
{quote}
Maybe, what we can do is that composite node store always observes events from underlying
stores. When an event is dispatched in same call as merge then composite node store ignores
the event. Else, it dispatches the event further up (maybe some massaging of the incoming
even be required).
{quote}

The reason I chose to not simply proxy-ied event from {{globalStore}} in {{merge}} as well
was because of a few reasons:
* it means that observation events would get dispatcher earlier (during merge of globalStore)
* observer event need to factor in complete root state - I don't know if it would really be
expected to use new revision from globalStore and merge them with "older" rev from other store
* I don't know if some other changes could be done beyond a merge into globalStore (not sure
of expectation of bottom half of merge code - hook and builder going into other stores)
* while it's still not handles afaics, but I think observation event should indeed get dispatched
as the last thing on a merge - otherwise, what is the expectation if an exception gets throw
after observation events have been sent

Btw, I also switched to {{ChangeDispatcher}} instead of managing own list of observers. Basically,
my intent was to get synchronized callbacks from {{merge}} and say potential {{backgroundRead}}
from global document store. But having non-duplicate was a nice plus as well :).

With all those comments, the current implementation has at least a few concerns that I don't
know how to really resolve - the primary issue is "how to manage order of events sent to observers".
Consider what happens in normal doc-store case for following events:
# some local commit R1 => observer gets R1
# background read to R2 => observer gets R2

Otoh, with current patch, it could happen:
# some local commit R1 starts and makes into doc-store
## doc-store dispatches obs event but no one gets it (since we're inside a merge call)
# doc-store runs a background read for R2-ds
## this gets dispatched to observer after aggregating current root states of other stores
- let's call this R2 to match remote rev in earlier series
# local commit of R1 completes => observers get R1

Clearly after the whole series the observers would actually be "feeling" to be sitting at
R1 which is wrong. The only 2 ways I can imagine to work around this:
# take {{mergeLock}} around dispatching external events
#* but, afaict, this can lead to a deadlock for seq2 above - step1 would've acquired the lock
and hence would block the background read dispatching events in step2
# resolve my doubts stated at top and act as simple proxy for events dispatched from globalStore

[~mreutegg], [~tomek.rekawek], it'd great to hear your thoughts around this. 

> CompositeNodeStore does not dispatch external events to observers
> -----------------------------------------------------------------
>
>                 Key: OAK-7710
>                 URL: https://issues.apache.org/jira/browse/OAK-7710
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: composite
>            Reporter: Vikas Saurabh
>            Priority: Major
>             Fix For: 1.10
>
>         Attachments: OAK-7710.patch, OAK-7710.test.patch
>
>
> Currently {{CompositeNodeStore}} only ever dispatches changes from inside its {{merge}}
method. This then loses external events that could be read in background read of some underlying
{{DocumentNodeStore}}.
> [^OAK-7710.test.patch] has an ignored test representing the scenario.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message