stefan-egli commented on code in PR #1317:
URL: https://github.com/apache/jackrabbit-oak/pull/1317#discussion_r1505997636
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Commit.java:
##########
@@ -354,6 +358,39 @@ private void applyToDocumentStore(RevisionVector
baseBranchRevision)
boolean commitRootHasChanges = operations.containsKey(commitRootPath);
for (UpdateOp op : operations.values()) {
NodeDocument.setCommitRoot(op, revision, commitRootDepth);
+
+ // special case for :childOrder updates
+ if (nodeStore.isChildOrderCleanupEnabled()) {
+ final Branch localBranch = getBranch();
+ if (localBranch != null) {
+ final NavigableSet<Revision> commits = new
TreeSet<>(localBranch.getCommits());
+ boolean removePreviousSetOperations = false;
+ for (Map.Entry<Key, Operation> change :
op.getChanges().entrySet()) {
+ if (":childOrder".equals(change.getKey().getName()) &&
Operation.Type.SET_MAP_ENTRY == change.getValue().type) {
Review Comment:
Ideally I guess there would be some oak-wide static for `:childOrder` - but
as it breaks bundle barriers that might be too much effort. Wondering if we
should have a static in `Commit` at least rather than having it in this method
twice?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Commit.java:
##########
@@ -354,6 +358,39 @@ private void applyToDocumentStore(RevisionVector
baseBranchRevision)
boolean commitRootHasChanges = operations.containsKey(commitRootPath);
for (UpdateOp op : operations.values()) {
NodeDocument.setCommitRoot(op, revision, commitRootDepth);
+
+ // special case for :childOrder updates
+ if (nodeStore.isChildOrderCleanupEnabled()) {
+ final Branch localBranch = getBranch();
+ if (localBranch != null) {
+ final NavigableSet<Revision> commits = new
TreeSet<>(localBranch.getCommits());
+ boolean removePreviousSetOperations = false;
+ for (Map.Entry<Key, Operation> change :
op.getChanges().entrySet()) {
+ if (":childOrder".equals(change.getKey().getName()) &&
Operation.Type.SET_MAP_ENTRY == change.getValue().type) {
+ // we are setting child order, so we should remove
previous set operations from the same branch
+ removePreviousSetOperations = true;
+ // branch.getCommits contains all revisions of the
branch
+ // including the new one we're about to make
+ // so don't do a removeMapEntry for that
+
commits.remove(change.getKey().getRevision().asBranchRevision());
+ }
+ }
+ if (removePreviousSetOperations) {
+ if (!commits.isEmpty()) {
+ int countRemoves = 0;
+ for (Revision rev : commits.descendingSet()) {
+ op.removeMapEntry(":childOrder",
rev.asTrunkRevision());
+ if (++countRemoves >= 256) {
Review Comment:
Should we also move those `256` to a static (which makes it more visible)?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java:
##########
@@ -869,6 +872,11 @@ public int getMemory() {
}
}
+
+ public boolean isChildOrderCleanupEnabled() {
+ return noChildOrderCleanupFeature == null ||
!noChildOrderCleanupFeature.isEnabled();
Review Comment:
Having second thought on the default being `true` .. we should play it more
safe by using `false` but aim to change it to `true` once we have seen it
_behave_ well in productive systems?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Commit.java:
##########
@@ -354,6 +358,39 @@ private void applyToDocumentStore(RevisionVector
baseBranchRevision)
boolean commitRootHasChanges = operations.containsKey(commitRootPath);
for (UpdateOp op : operations.values()) {
NodeDocument.setCommitRoot(op, revision, commitRootDepth);
+
+ // special case for :childOrder updates
+ if (nodeStore.isChildOrderCleanupEnabled()) {
+ final Branch localBranch = getBranch();
+ if (localBranch != null) {
+ final NavigableSet<Revision> commits = new
TreeSet<>(localBranch.getCommits());
+ boolean removePreviousSetOperations = false;
+ for (Map.Entry<Key, Operation> change :
op.getChanges().entrySet()) {
+ if (":childOrder".equals(change.getKey().getName()) &&
Operation.Type.SET_MAP_ENTRY == change.getValue().type) {
+ // we are setting child order, so we should remove
previous set operations from the same branch
+ removePreviousSetOperations = true;
+ // branch.getCommits contains all revisions of the
branch
+ // including the new one we're about to make
+ // so don't do a removeMapEntry for that
+
commits.remove(change.getKey().getRevision().asBranchRevision());
+ }
+ }
+ if (removePreviousSetOperations) {
+ if (!commits.isEmpty()) {
+ int countRemoves = 0;
+ for (Revision rev : commits.descendingSet()) {
+ op.removeMapEntry(":childOrder",
rev.asTrunkRevision());
+ if (++countRemoves >= 256) {
+ LOG.debug("applyToDocumentStore : only
cleaning up last {} branch commits.",
+ countRemoves);
+ break;
+ }
+ }
+ LOG.debug("applyToDocumentStore :
childOrder-edited op is: " + op);
Review Comment:
```suggestion
LOG.debug("applyToDocumentStore :
childOrder-edited op is: {}", op);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
