[jira] [Comment Edited] (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 edited comment on OAK-4916 at 10/13/16 4:00 PM:
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.
Assuming OAK-4898 and passing the FilterProvider doesn't make it into 1.6 I'm
wondering if this should be added as a known-issue to
https://jackrabbit.apache.org/oak/docs/known_issues.html
was (Author: egli):
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] [Comment Edited] (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=15572165#comment-15572165
]
Stefan Egli edited comment on OAK-4916 at 10/13/16 3:19 PM:
Attaching [^OAK-4916.v2.patch] which is an alternative variant of filtering in
the BackgroundObserver:
* as suggested by Chetan introduced a {{FilteringAwareObserver}} which is
implemented by BackgroundObserver and thus moves the filtering _decision_ to an
upstream Observer. Thus an element doesn't even get passed to the
BackgroundObserver if it is filtered. However, what the BackgroundObserver must
do is to correctly pass this exclusion to its downstream Observer - and it must
take special care when the queue is full: as basically then exclusion doesn't
work.
* added a base class for doing the upstream filtering in {{FilteringObserver}}
* added several tests to the existing BackgroundObserverTest as well as added a
new PrefilteringBackgroundObserverTest.
* NOTE: downstream observers of a BackgroundObserver should now implement
FilteringAwareObserver in order to correctly get the filtering signal. If they
don't they will get a normal contentChanged however they then loose the
CommitInfo (and the commit-boundary might be wrong). Ie a BackgroundObserver
shouldn't be used in a chain where filtering is applied (ie where there is an
upstream FilteringObserver) to avoid lossing the CommitInfo.
[~chetanm], [~mduerig], wdyt?
was (Author: egli):
Attaching [^OAK-4916.v2.patch] which is an alternative variant of filtering in
the BackgroundObserver:
* as suggested by Chetan introduced a {{FilteringAwareObserver}} which is
implemented by BackgroundObserver and thus moves the filtering _decision_ to an
upstream Observer. Thus an element doesn't even get passed to the
BackgroundObserver if it is filtered. However, what the BackgroundObserver must
do is to correctly pass this exclusion to its downstream Observer - and it must
take special care when the queue is full: as basically then exclusion doesn't
work.
* added a base class for doing the upstream filtering in {{FilteringObserver}}
* added several tests to the existing BackgroundObserverTest as well as added a
new PrefilteringBackgroundObserverTest.
* NOTE: downstream observers of a BackgroundObserver should now implement
FilteringAwareObserver in order to correctly get the filtering signal. If they
don't they will get a normal contentChanged however they then loose the
CommitInfo. Ie a BackgroundObserver shouldn't be used in a chain where
filtering is applied (ie where there is an upstream FilteringObserver) to avoid
lossing the CommitInfo.
[~chetanm], [~mduerig], wdyt?
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Comment Edited] (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=15572165#comment-15572165
]
Stefan Egli edited comment on OAK-4916 at 10/13/16 3:03 PM:
Attaching [^OAK-4916.v2.patch] which is an alternative variant of filtering in
the BackgroundObserver:
* as suggested by Chetan introduced a {{FilteringAwareObserver}} which is
implemented by BackgroundObserver and thus moves the filtering _decision_ to an
upstream Observer. Thus an element doesn't even get passed to the
BackgroundObserver if it is filtered. However, what the BackgroundObserver must
do is to correctly pass this exclusion to its downstream Observer - and it must
take special care when the queue is full: as basically then exclusion doesn't
work.
* added a base class for doing the upstream filtering in {{FilteringObserver}}
* added several tests to the existing BackgroundObserverTest as well as added a
new PrefilteringBackgroundObserverTest.
* NOTE: downstream observers of a BackgroundObserver should now implement
FilteringAwareObserver in order to correctly get the filtering signal. If they
don't they will get a normal contentChanged however they then loose the
CommitInfo. Ie a BackgroundObserver shouldn't be used in a chain where
filtering is applied (ie where there is an upstream FilteringObserver) to avoid
lossing the CommitInfo.
[~chetanm], [~mduerig], wdyt?
was (Author: egli):
Attaching [^OAK-4916.v2.patch] which is an alternative variant of filtering in
the BackgroundObserver:
* as suggested by Chetan introduced a {{FilteringAwareObserver}} which is
implemented by BackgroundObserver and thus moves the filtering _decision_ to an
upstream Observer. Thus an element doesn't even get passed to the
BackgroundObserver if it is filtered. However, what the BackgroundObserver must
do is to correctly pass this exclusion to its downstream Observer - and it must
take special care when the queue is full: as basically then exclusion doesn't
work.
* added a base class for doing the upstream filtering in {{FilteringObserver}}
* added several tests to the existing BackgroundObserverTest as well as added a
new PrefilteringBackgroundObserverTest.
[~chetanm], [~mduerig], wdyt?
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Comment Edited] (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 edited comment on OAK-4916 at 10/12/16 7:39 AM:
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. This is similar to current proposed approach
just using 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 ;)
was (Author: chetanm):
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
