[
https://issues.apache.org/jira/browse/OAK-4907?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15567808#comment-15567808
]
Chetan Mehrotra commented on OAK-4907:
--------------------------------------
Patch looks good and test coverage is good!.
Some minor comments
# Instead of special casing for EMPTY it would be better to check if the
{{CommitInfo}} has {{CommitContext}} set. If yes then validator can opt in
otherwise opt out. And then {{CollectorSupport}} can just refer to
{{CommitContext}}
{code}
if (info == null || info == CommitInfo.EMPTY) {
// then we cannot do change-collecting, as we can't store
// it in the info
return null;
}
{code}
# I see following 3 boolean vars always being set to true. May be replace 3
with single var like {{changed}}
{code}
// record a change at the current path (which is the
parent of this property)
addParentPathOnLeave = true;
// and with the current node name
addParentNodeNameOnLeave = true;
// additionally, add parent's nodeType on leave too
addParentNodeTypeOnLeave = true;
{code}
# Log the limits/config at info level in activate method
# Keep the child name as instance variable to avoid computing it again
{code}
if (addParentNodeNameOnLeave) {
support.getParentNodeNames().add(getName(path));
}
{code}
# Use {{Iterables#addAll}}. This avoid creating unnecessary list instance and
is bit optimized for collection based iterable (which is the case here). Doing
all this would ensure that overhead of validator is minimal!
{code}
support.getParentNodeTypes().addAll(Lists.newArrayList(parentNodeOrNull.getNames("jcr:mixinTypes")));
{code}
# {{testNull}} test is failing for me
h4. ChangeSet
May be we should split this class in two.
*ChangeSetBuilder*
{code}
public class ChangeSetBuilder {
private final int maxItems;
private final int maxPathDepth;
private final Set<String> parentPaths = Sets.newHashSet();
private boolean parentPathOverflow;
public ChangeSetBuilder(int maxItems, int maxPathDepth) {
this.maxItems = maxItems;
this.maxPathDepth = maxPathDepth;
}
public ChangeSetBuilder addParentPath(String path){
if (parentPaths.size() > maxItems){
parentPathOverflow = true;
parentPaths.clear();
}
if (!parentPathOverflow) {
parentPaths.add(path);
}
return this;
}
public ChangeSet build(){
return new ChangeSet(...)
}
}
{code}
This class
* Just collects the changes and take care of limits
* Is mutable
* Used by Validator and not to be used by clients
*ChangeSet*
{code}
public class ChangeSet {
@CheckForNull
public Set<String> getParentPaths() {
}
}
{code}
* Immutable class
* Has accessors marked with {{CheckForNull}}. With null being an indication
that this information was not collected and observer should do the hard work
{noformat}
* To limit memory usage, the ChangeSet has a limit on the number of items,
each,
* that it collects. If one of those items reach the limit this is called
* an 'overflow' and the corresponding item type is marked as having
'overflown'.
* Downstream Observers should thus check if a particular item has overflown
* or not.
{noformat}
This can be updated to indicate "how" overflow case can be determined i.e. by
checking if the set returned is null
Also some coverage around overflow case would be good to have
> Collect changes (paths, nts, props..) of a commit in a validator
> ----------------------------------------------------------------
>
> Key: OAK-4907
> URL: https://issues.apache.org/jira/browse/OAK-4907
> 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-4907.patch, OAK-4907.v2.patch
>
>
> It would be useful to collect a set of changes of a commit (eg in a
> validator) that could later be used in an Observer for eg prefiltering.
> Such a change collector should collect paths, nodetypes, properties,
> node-names (and perhaps more at a later stage) of all changes and store the
> result in the CommitInfo's CommitContext.
> Note that this is a result of
> [discussions|https://issues.apache.org/jira/browse/OAK-4796?focusedCommentId=15550962&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15550962]
> around design in OAK-4796
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
