[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15628948#comment-15628948 ] Stefan Egli commented on OAK-4916: -- tests will follow in OAK-4908 > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.5.13 > > Attachments: FilteringObserver.patch, OAK-4916.patch, > OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15628927#comment-15628927 ] Stefan Egli commented on OAK-4916: -- Committed a modified variant (mainly with separate Interfaces for the different concerns) of [~mreutegg]'s patch [here|http://svn.apache.org/viewvc?rev=1767668=rev] > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: FilteringObserver.patch, OAK-4916.patch, > OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15601543#comment-15601543 ] Stefan Egli commented on OAK-4916: -- Ok, after another review and an offline discussion I believe this is an elegant version as it indeed leaves the BackgroundObserver untouched. I would suggest one change which is to use an {{abstract boolean isFiltered(...)}} rather than a Predicate. I'll look into rewriting the patch based on this FilteringObserver. > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: FilteringObserver.patch, OAK-4916.patch, > OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15601297#comment-15601297 ] Marcel Reutegger commented on OAK-4916: --- The main difference is, it doesn't introduce the filtering concept in {{BackgroundObserver}} but uses it as is. This separates the two concerns and keeps the {{BackgroundObserver}} simple. > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: FilteringObserver.patch, OAK-4916.patch, > OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15601282#comment-15601282 ] Stefan Egli commented on OAK-4916: -- [~mreutegg], how do you see your patch compare to my [previous one|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15585445=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15585445] based on a {{FilteringAwareObserver}}? That one doesn't use NOOP_CHANGE anymore neither. I guess the main diff is that your variant has a {{contentChanged(@Nonnull NodeState before, @Nonnull NodeState after, @Nullable CommitInfo info)}} while the FilteringAwareObserver variant has a dedicated, separate {{void resetPreviousRoot(NodeState root);}} > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: FilteringObserver.patch, OAK-4916.patch, > OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15595549#comment-15595549 ] Marcel Reutegger commented on OAK-4916: --- See [^FilteringObserver.patch] for a quick sketch of my idea. > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: FilteringObserver.patch, OAK-4916.patch, > OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15595433#comment-15595433 ] Marcel Reutegger commented on OAK-4916: --- I would also prefer a compositional approach. I already find it difficult to understand what exactly {{BackgroundObserver.contentChanged()}} does right now. Adding more logic to it will probably make it more difficult to maintain and understand. I like the NOOP_CHANGED idea, but I would try to keep it an implementation detail. That means we'd need to introduce a new kind of observer, which allows filtering of some contentChanged() calls. This new type of observer would not just get the new root state, but also the base state it needs to compare against. In between we'd have our new filtering observer that supports queuing with the help of a BackgroundObserver, but also pre-filters incoming changes, collapses multiple filtered changes into a NOOP_CHANGED and passes it to the BackgroundObserver. The latter doesn't need to understand the semantics of NOOP_CHANGED. But it would be recognized on the consumer side of the new filtering observer. I'll try to sketch this idea in a patch. > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: OAK-4916.patch, OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15587958#comment-15587958 ] Stefan Egli commented on OAK-4916: -- [~chetanm], sure, no problem. I'll also look at the benchmark one of the next days. > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: OAK-4916.patch, OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15587657#comment-15587657 ] Chetan Mehrotra commented on OAK-4916: -- [~egli] I would like to review the patch closely but need some more time (2 days?) as wrapping the node bundling feature. Given [~mduerig] implemented most of these parts it would be helpful if he review this change (and also one in OAK-4908) In the meantime it would be useful if we can run the {{org.apache.jackrabbit.oak.benchmark.ObservationTest}} benchmark that [~mreutegg] implemented with all these new support. Benchmark would need some change to ensure that certain commits done are of interest to observer and certain are just to be filtered out > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: OAK-4916.patch, OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15585445#comment-15585445 ] Stefan Egli commented on OAK-4916: -- Assuming approach in [^OAK-4916.v2.patch] is fine I plan to commit this tomorrow morning. > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: OAK-4916.patch, OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15572294#comment-15572294 ] Stefan Egli commented on OAK-4916: -- Note that the issue with switching a filter [mentioned here|https://issues.apache.org/jira/browse/OAK-4796?focusedCommentId=15548765=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15548765] still applies. It would require OAK-4898 to pass the FilterProvider with each CommitInfo into the BackgroundObserver's queue for this to work correctly. > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: OAK-4916.patch, OAK-4916.v2.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. > NOTE: See [comment further > below|https://issues.apache.org/jira/browse/OAK-4916?focusedCommentId=15572165=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15572165] > with a new suggested approach, which doesn't use NOOP_CHANGED but introduces > a new FilteringAwareObserver instead. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15568342#comment-15568342 ] Stefan Egli commented on OAK-4916: -- One way to do this would be to make the {{BackgroundObserver}} a {{FilteringAwareObserver}} such that it could get the 'skip' signal from the upstream, new '{{FilteringObserver}}'. It would be another variant to what Chetan's composition pattern, however the BackgroundObserver would still have to maintain about the same logic in {{contentChanged}} wrt skipped changes as now: the goal of prefiltering is to shrink the queue of the BackgroundObserver and for that we do have to make some adaptions there. Surely there are other variants than what I came up with, but I doubt it's possible without any change in the BackgroundObserver. I'll work on a next iteration based on these 2 feedbacks, Thx! > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: OAK-4916.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15567975#comment-15567975 ] Michael Dürig commented on OAK-4916: Without looking at the code very much, it seems we are overloading {{BackgroundObserver}} with too many concerns. That class should only be concerned about turning a synchronous observer into an asynchronous one. I would prefer a compositional approach here where a pre filtering observer would synchronously see all content changes and pass the filtered ones along to the background observer. > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: OAK-4916.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (OAK-4916) Add support for excluding commits to BackgroundObserver
[ https://issues.apache.org/jira/browse/OAK-4916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15567937#comment-15567937 ] Chetan Mehrotra commented on OAK-4916: -- Somehow not very comfortable with {{NOOP_CHANGE}} based protocol for communicating filtering. May be we introduce new interfaces # ContentChangeFilter - This would be passed to {{BackgroundObserver}} and would be responsible for determining if given change should be included in the queue or not. By default {{BackgroundObserver}} would {{ContentChangeFilter#DEFAULT}}. Observers which support prefiltering would provide there own implementation. Kind of composition over inheritence {code} public interface ContentChangeFilter { ContentChangeFilter DEFAULT = new ContentChangeFilter() { @Override public boolean includeChange(CommitInfo info) { return true } }; boolean includeChange(CommitInfo info); } {code} # FilteringAwareObserver - Any observer which allows prefiltering would also implement this interface. And instead of {{NOOP_CHANGE}} based convention {{BackgroundObserver}} would invoke the {{resetPreviousRoot}} to indicate that base root state needs to be adjusted. {code} public interface FilteringAwareObserver extends Observer{ /** * Invoked to enable such observers to reset there previous root * to given NodeState * * @param root previous NodeState root */ void resetPreviousRoot(NodeState root); } {code} Another approach would be to make such observer state less i.e. they do not track the previous state and we pass in previous state and {{BackgroundObserver}} maintains a tuple {code} void contentChanged(@Nullable NodeState before, @Nonnull NodeState after, @Nullable CommitInfo info); {code} [~mduerig] Would have better thoughts here ;) > Add support for excluding commits to BackgroundObserver > --- > > Key: OAK-4916 > URL: https://issues.apache.org/jira/browse/OAK-4916 > Project: Jackrabbit Oak > Issue Type: Technical task > Components: core >Affects Versions: 1.5.11 >Reporter: Stefan Egli >Assignee: Stefan Egli > Fix For: 1.6 > > Attachments: OAK-4916.patch > > > As part of pre-filtering commits it would be useful to have support in the > BackgroundObserver (in general) that would allow to exclude certain commits > from being added to the (BackgroundObserver's) queue, thus keeping the queue > smaller. The actual filtering is up to subclasses. > The suggested implementation is as follows: > * a new method {{isExcluded}} is introduced which represents a subclass hook > for filtering > * excluded commits are not added to the queue > * when multiple commits are excluded subsequently, this is collapsed > * the first non-excluded commit (ContentChange) added to the queue is marked > with the last non-excluded root state as the 'previous root' > * downstream Observers are notified of the exclusion of a commit via a > special CommitInfo {{NOOP_CHANGE}}: this instructs it to exclude this change > while at the same time 'fast-forwarding' the root state to the new one. > ** this extra token is one way of solving the problem that > {{Observer.contentChanged}} represents a diff between two states but does not > transport the 'from' state explicitly - that is implicitly taken from the > previous call to {{contentChanged}}. Thus using such a gap token > ({{NOOP_CHANGE}}) seems to be the only way to instruct Observers to skip a > change. > To repeat: whoever extends BackgroundObserver with filtering must be aware of > the new {{NOOP_CHANGE}} token. Anyone not doing filtering will not get any > {{NOOP_CHANGE}} tokens though. -- This message was sent by Atlassian JIRA (v6.3.4#6332)