Index: src/main/java/org/apache/jackrabbit/oak/plugins/document/Branch.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/document/Branch.java (revision 1642837) +++ src/main/java/org/apache/jackrabbit/oak/plugins/document/Branch.java (revision ) @@ -192,7 +192,8 @@ /** * Gets the most recent unsaved last revision at readRevision - * or earlier in this branch for the given path. + * or earlier in this branch for the given path. Documents with + * explicit updates are not tracked and this method may return {@code null}. * * @param path the path of a node. * @param readRevision the read revision. Index: src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java (revision 1643106) +++ src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java (revision ) @@ -60,6 +60,7 @@ import static org.apache.jackrabbit.oak.plugins.document.UpdateOp.Key; import static org.apache.jackrabbit.oak.plugins.document.UpdateOp.Operation; import static org.apache.jackrabbit.oak.plugins.document.util.Utils.isRevisionNewer; +import static org.apache.jackrabbit.oak.plugins.document.util.Utils.resolveCommitRevision; /** * A document storing data about a node. @@ -160,7 +161,7 @@ /** * Whether this node is deleted. Key: revision, value: true/false. */ - private static final String DELETED = "_deleted"; + static final String DELETED = "_deleted"; /** * Flag indicating that whether this node was ever deleted. @@ -764,7 +765,12 @@ @Nonnull Revision readRevision, @Nullable Revision lastModified) { Map validRevisions = Maps.newHashMap(); - Revision min = getLiveRevision(nodeStore, readRevision, validRevisions); + Branch branch = nodeStore.getBranches().getBranch(readRevision); + LastRevs lastRevs = new LastRevs(getLastRev(), readRevision, branch); + // overlay with unsaved last modified from this instance + lastRevs.update(lastModified); + + Revision min = getLiveRevision(nodeStore, readRevision, validRevisions, lastRevs); if (min == null) { // deleted return null; @@ -778,7 +784,7 @@ } // first check local map, which contains most recent values Value value = getLatestValue(nodeStore, getLocalMap(key), - min, readRevision, validRevisions); + min, readRevision, validRevisions, lastRevs); // check if there may be more recent values in a previous document if (value != null && !getPreviousRanges().isEmpty()) { @@ -798,7 +804,7 @@ if (value == null && !getPreviousRanges().isEmpty()) { // check complete revision history value = getLatestValue(nodeStore, getValueMap(key), - min, readRevision, validRevisions); + min, readRevision, validRevisions, lastRevs); } String propertyName = Utils.unescapePropertyName(key); String v = value != null ? value.value : null; @@ -815,17 +821,12 @@ // _lastRev. // when was this node last modified? - Branch branch = nodeStore.getBranches().getBranch(readRevision); - Map lastRevs = Maps.newHashMap(getLastRev()); - // overlay with unsaved last modified from this instance - if (lastModified != null) { - lastRevs.put(nodeStore.getClusterId(), lastModified); - } Revision branchBase = null; if (branch != null) { branchBase = branch.getBase(readRevision); } - for (Revision r : lastRevs.values()) { + for (Revision r : lastRevs.get().values()) { + // TODO: rather if (branchBase != null) {} else {} // ignore if newer than readRevision if (isRevisionNewer(nodeStore, r, readRevision)) { // the node has a _lastRev which is newer than readRevision @@ -854,9 +855,10 @@ if (branch != null) { // read from a branch // -> possibly overlay with unsaved last revs from branch - Revision r = branch.getUnsavedLastRevision(path, readRevision); + lastRevs.updateBranch(branch.getUnsavedLastRevision(path, readRevision)); + Revision r = lastRevs.getBranchRevision(); if (r != null) { - lastRevision = r.asBranchRevision(); + lastRevision = r; } } n.setLastRevision(lastRevision); @@ -871,19 +873,21 @@ * @param maxRev the maximum revision to return * @param validRevisions the map of revisions to commit value already * checked against maxRev and considered valid. + * @param lastRevs to keep track of the last modification. * @return the earliest revision, or null if the node is deleted at the * given revision */ @CheckForNull public Revision getLiveRevision(RevisionContext context, Revision maxRev, - Map validRevisions) { + Map validRevisions, + LastRevs lastRevs) { // check local deleted map first Value value = getLatestValue(context, getLocalDeleted(), - null, maxRev, validRevisions); + null, maxRev, validRevisions, lastRevs); if (value == null && !getPreviousRanges().isEmpty()) { // need to check complete map value = getLatestValue(context, getDeleted(), - null, maxRev, validRevisions); + null, maxRev, validRevisions, lastRevs); } return value != null && "false".equals(value.value) ? value.revision : null; @@ -1168,16 +1172,6 @@ revision.toString()); } - public static boolean hasLastRev(@Nonnull UpdateOp op, int clusterId) { - return checkNotNull(op).getChanges().containsKey( - new Key(LAST_REV, new Revision(0, 0, clusterId))); - } - - public static void unsetLastRev(@Nonnull UpdateOp op, int clusterId) { - checkNotNull(op).unsetMapEntry(LAST_REV, - new Revision(0, 0, clusterId)); - } - public static void setCommitRoot(@Nonnull UpdateOp op, @Nonnull Revision revision, int commitRootDepth) { @@ -1355,7 +1349,7 @@ if (context.getBranches().getBranch(readRevision) == null && !readRevision.isBranch()) { // resolve commit revision - revision = Utils.resolveCommitRevision(revision, commitValue); + revision = resolveCommitRevision(revision, commitValue); // readRevision is not from a branch // compare resolved revision as is return !isRevisionNewer(context, revision, readRevision); @@ -1374,7 +1368,7 @@ return false; } } - return includeRevision(context, Utils.resolveCommitRevision(revision, commitValue), readRevision); + return includeRevision(context, resolveCommitRevision(revision, commitValue), readRevision); } /** @@ -1437,6 +1431,7 @@ * @param readRevision the maximum revision * @param validRevisions map of revision to commit value considered valid * against the given readRevision. + * @param lastRevs to keep track of the most recent modification. * @return the value, or null if not found */ @CheckForNull @@ -1444,14 +1439,10 @@ @Nonnull Map valueMap, @Nullable Revision min, @Nonnull Revision readRevision, - @Nonnull Map validRevisions) { + @Nonnull Map validRevisions, + @Nonnull LastRevs lastRevs) { for (Map.Entry entry : valueMap.entrySet()) { Revision propRev = entry.getKey(); - // ignore revisions newer than readRevision - // -> these are not visible anyway - if (isRevisionNewer(context, propRev, readRevision)) { - continue; - } String commitValue = validRevisions.get(propRev); if (commitValue == null) { // resolve revision @@ -1464,15 +1455,21 @@ continue; } } - if (min != null && isRevisionNewer(context, min, - Utils.resolveCommitRevision(propRev, commitValue))) { + + Revision commitRev = resolveCommitRevision(propRev, commitValue); + if (Utils.isCommitted(commitValue)) { + lastRevs.update(commitRev); + } else { + // branch commit + lastRevs.updateBranch(commitRev.asBranchRevision()); + } + + if (min != null && isRevisionNewer(context, min, commitRev)) { continue; } if (isValidRevision(context, propRev, commitValue, readRevision, validRevisions)) { // TODO: need to check older revisions as well? - return new Value( - Utils.resolveCommitRevision(propRev, commitValue), - entry.getValue()); + return new Value(commitRev, entry.getValue()); } } return null; Index: src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgentTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgentTest.java (revision 1642837) +++ src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgentTest.java (revision ) @@ -127,8 +127,7 @@ b2.child("x").child("y").child("z").setProperty("foo", "bar"); ds2.merge(b2, EmptyHook.INSTANCE, CommitInfo.EMPTY); - NodeDocument z1 = getDocument(ds1, "/x/y/z"); - Revision zlastRev2 = z1.getLastRev().get(c2Id); + Revision zlastRev2 = ds2.getHeadRevision(); long leaseTime = ds1.getClusterInfo().getLeaseTime(); ds1.runBackgroundOperations(); Index: src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java (revision 1642837) +++ src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java (revision ) @@ -38,6 +38,7 @@ import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.plugins.document.Revision; import org.apache.jackrabbit.oak.plugins.document.RevisionContext; +import org.apache.jackrabbit.oak.plugins.document.StableRevisionComparator; import org.bson.types.ObjectId; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -460,6 +461,25 @@ @Nonnull Revision x, @Nonnull Revision previous) { return context.getRevisionComparator().compare(x, previous) > 0; + } + + /** + * Returns the revision with the newer timestamp or {@code null} if both + * revisions are {@code null}. The implementation will return the first + * revision if both have the same timestamp. + * + * @param a the first revision (or {@code null}). + * @param b the second revision (or {@code null}). + * @return the revision with the newer timestamp. + */ + @CheckForNull + public static Revision max(@Nullable Revision a, @Nullable Revision b) { + if (a == null) { + return b; + } else if (b == null) { + return a; + } + return StableRevisionComparator.INSTANCE.compare(a, b) >= 0 ? a : b; } } Index: src/main/java/org/apache/jackrabbit/oak/plugins/document/Commit.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/document/Commit.java (revision 1642837) +++ src/main/java/org/apache/jackrabbit/oak/plugins/document/Commit.java (revision ) @@ -36,8 +36,10 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.apache.jackrabbit.oak.commons.PathUtils.denotesRoot; import static org.apache.jackrabbit.oak.plugins.document.Collection.NODES; import static org.apache.jackrabbit.oak.plugins.document.NodeDocument.COLLISIONS; +import static org.apache.jackrabbit.oak.plugins.document.NodeDocument.REVISIONS; import static org.apache.jackrabbit.oak.plugins.document.NodeDocument.SPLIT_CANDIDATE_THRESHOLD; /** @@ -256,7 +258,7 @@ } else { while (!PathUtils.isAncestor(commitRootPath, p)) { commitRootPath = PathUtils.getParentPath(commitRootPath); - if (PathUtils.denotesRoot(commitRootPath)) { + if (denotesRoot(commitRootPath)) { break; } } @@ -280,12 +282,6 @@ } else { NodeDocument.setCommitRoot(op, revision, commitRootDepth); if (op.isNew()) { - if (baseBranchRevision == null) { - // for new non-branch nodes we can safely set _lastRev on - // insert. for existing nodes the _lastRev is updated by - // the background thread to avoid concurrent updates - NodeDocument.setLastRev(op, revision); - } newNodes.add(op); } else { changedNodes.add(op); @@ -297,7 +293,6 @@ // it is the root of a subtree added in a commit. // so we try to add the root like all other nodes NodeDocument.setRevision(commitRoot, revision, commitValue); - NodeDocument.setLastRev(commitRoot, revision); newNodes.add(commitRoot); } try { @@ -312,9 +307,6 @@ // (because there might be a conflict) NodeDocument.unsetRevision(commitRoot, revision); } - // setting _lastRev is only safe on insert. now the - // background thread needs to take care of it - NodeDocument.unsetLastRev(op, revision.getClusterId()); changedNodes.add(op); } newNodes.clear(); @@ -381,7 +373,7 @@ private void updateParentChildStatus() { final Set processedParents = Sets.newHashSet(); for (String path : addedNodes) { - if (PathUtils.denotesRoot(path)) { + if (denotesRoot(path)) { continue; } @@ -407,7 +399,6 @@ } for (UpdateOp op : newDocuments) { UpdateOp reverse = op.getReverseOperation(); - NodeDocument.unsetLastRev(reverse, revision.getClusterId()); store.findAndUpdate(NODES, reverse); } UpdateOp removeCollision = new UpdateOp(commitRoot.getId(), false); @@ -542,7 +533,7 @@ public void applyToCache(Revision before, boolean isBranchCommit) { HashMap> nodesWithChangedChildren = new HashMap>(); for (String p : modifiedNodes) { - if (PathUtils.denotesRoot(p)) { + if (denotesRoot(p)) { continue; } String parent = PathUtils.getParentPath(p); @@ -554,6 +545,7 @@ list.add(p); } DiffCache.Entry cacheEntry = nodeStore.getDiffCache().newEntry(before, revision); + LastRevTracker tracker = nodeStore.createTracker(revision, isBranchCommit); List added = new ArrayList(); List removed = new ArrayList(); List changed = new ArrayList(); @@ -575,11 +567,13 @@ } UpdateOp op = operations.get(path); boolean isNew = op != null && op.isNew(); - boolean pendingLastRev = op == null - || !NodeDocument.hasLastRev(op, revision.getClusterId()); - nodeStore.applyChanges(revision, path, isNew, pendingLastRev, - isBranchCommit, added, removed, changed, cacheEntry); + if (op == null || !hasContentChanges(op) || denotesRoot(path)) { + // track intermediate node and root + tracker.track(path); - } + } + nodeStore.applyChanges(revision, path, isNew, + added, removed, changed, cacheEntry); + } cacheEntry.done(); } @@ -592,14 +586,14 @@ } private void markChanged(String path) { - if (!PathUtils.denotesRoot(path) && !PathUtils.isAbsolute(path)) { + if (!denotesRoot(path) && !PathUtils.isAbsolute(path)) { throw new IllegalArgumentException("path: " + path); } while (true) { if (!modifiedNodes.add(path)) { break; } - if (PathUtils.denotesRoot(path)) { + if (denotesRoot(path)) { break; } path = PathUtils.getParentPath(path); @@ -621,4 +615,13 @@ NodeDocument.setDeleted(op, revision, true); } + private static boolean hasContentChanges(UpdateOp op) { + for (UpdateOp.Key key : op.getChanges().keySet()) { + if (Utils.isPropertyName(key.getName()) + || NodeDocument.DELETED.equals(key.getName())) { + return true; + } + } + return false; + } } Index: src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevTest.java (revision ) +++ src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevTest.java (revision ) @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.plugins.document; + +import org.apache.jackrabbit.oak.spi.commit.CommitInfo; +import org.apache.jackrabbit.oak.spi.commit.EmptyHook; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.junit.Test; + +import static org.apache.jackrabbit.oak.plugins.document.Collection.NODES; +import static org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + +/** + * Checks if _lastRev entries are correctly set and updated. + */ +public class LastRevTest { + + @Test + public void lastRev() throws Exception { + DocumentNodeStore store = new DocumentMK.Builder() + .setAsyncDelay(0).getNodeStore(); + DocumentStore docStore = store.getDocumentStore(); + + NodeBuilder root = store.getRoot().builder(); + for (int i = 0; i < 10; i++) { + NodeBuilder child = root.child("child-" + i); + for (int j = 0; j < 10; j++) { + child.child("test-" + j); + } + } + store.merge(root, EmptyHook.INSTANCE, CommitInfo.EMPTY); + store.runBackgroundOperations(); + + for (int i = 0; i < 10; i++) { + String parentPath = "/child-" + i; + assertLastRevSize(docStore, parentPath, 0); + for (int j = 0; j < 10; j++) { + String path = parentPath + "/test-" + j; + assertLastRevSize(docStore, path, 0); + } + } + + store.dispose(); + } + + private static void assertLastRevSize(DocumentStore store, + String path, int size) { + NodeDocument doc = store.find(NODES, getIdFromPath(path)); + assertNotNull(doc); + assertEquals("_lastRev: " + doc.getLastRev(), size, doc.getLastRev().size()); + } + +} Index: src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java (revision 1642837) +++ src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java (revision ) @@ -253,11 +253,9 @@ String id = Utils.getIdFromPath("/foo/node"); NodeDocument doc = docStore.find(Collection.NODES, id); assertNotNull("document with id " + id + " does not exist", doc); - assertTrue(!doc.getLastRev().isEmpty()); id = Utils.getIdFromPath("/bar/node"); doc = docStore.find(Collection.NODES, id); assertNotNull("document with id " + id + " does not exist", doc); - assertTrue(!doc.getLastRev().isEmpty()); mk.dispose(); } @@ -393,30 +391,6 @@ nodeStore1.dispose(); nodeStore2.dispose(); nodeStore3.dispose(); - } - - // OAK-1820 - @Test - public void setLastRevOnCommitForNewNode() throws Exception { - DocumentNodeStore ns = new DocumentMK.Builder() - .setAsyncDelay(0).getNodeStore(); - // add a first child node. this will set the children flag on root - // and move the commit root to the root - NodeBuilder builder = ns.getRoot().builder(); - builder.child("foo"); - ns.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY); - - // the second time, the added node is also the commit root, this - // is the case we are interested in - builder = ns.getRoot().builder(); - builder.child("bar"); - ns.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY); - - NodeDocument doc = ns.getDocumentStore().find(NODES, - Utils.getIdFromPath("/bar")); - assertEquals(1, doc.getLastRev().size()); - - ns.dispose(); } @Test Index: src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryTest.java (revision 1642837) +++ src/test/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryTest.java (revision ) @@ -29,7 +29,6 @@ import org.junit.Test; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; public class LastRevRecoveryTest { @@ -87,7 +86,9 @@ NodeDocument x1 = getDocument(ds1, "/x"); Revision zlastRev2 = z1.getLastRev().get(c2Id); - assertNotNull(zlastRev2); + // /x/y/z is a new node and does not have a _lastRev + assertNull(zlastRev2); + Revision head2 = ds2.getHeadRevision(); //lastRev should not be updated for C #2 assertNull(y1.getLastRev().get(c2Id)); @@ -98,9 +99,9 @@ recovery.recover(Iterators.forArray(x1,z1), c2Id); //Post recovery the lastRev should be updated for /x/y and /x - assertEquals(zlastRev2, getDocument(ds1, "/x/y").getLastRev().get(c2Id)); - assertEquals(zlastRev2, getDocument(ds1, "/x").getLastRev().get(c2Id)); - assertEquals(zlastRev2, getDocument(ds1, "/").getLastRev().get(c2Id)); + assertEquals(head2, getDocument(ds1, "/x/y").getLastRev().get(c2Id)); + assertEquals(head2, getDocument(ds1, "/x").getLastRev().get(c2Id)); + assertEquals(head2, getDocument(ds1, "/").getLastRev().get(c2Id)); } private NodeDocument getDocument(DocumentNodeStore nodeStore, String path) { Index: src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevs.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevs.java (revision ) +++ src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevs.java (revision ) @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.plugins.document; + +import java.util.HashMap; +import java.util.Map; + +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +import org.apache.jackrabbit.oak.plugins.document.util.Utils; + +/** + * Helper class to track when a node was last modified. + */ +final class LastRevs { + + private final Map revs; + + private final Revision readRevision; + + private final Branch branch; + + private Revision branchRev; + + LastRevs(Map revs, Revision readRevision, Branch branch) { + this.revs = new HashMap(revs); + this.readRevision = readRevision; + this.branch = branch; + } + + void update(@Nullable Revision rev) { + if (rev == null) { + return; + } + Revision r = revs.get(rev.getClusterId()); + if (r == null || rev.compareRevisionTime(r) > 0) { + revs.put(rev.getClusterId(), rev); + } + } + + void updateBranch(@Nullable Revision rev) { + if (rev == null) { + return; + } + rev = rev.asBranchRevision(); + if (branch != null && branch.containsCommit(rev) + && readRevision.compareRevisionTime(rev) >= 0) { + branchRev = Utils.max(branchRev, rev); + } + } + + @CheckForNull + Revision getBranchRevision() { + return branchRev; + } + + @Nonnull + Map get() { + return revs; + } +} Index: src/test/java/org/apache/jackrabbit/oak/plugins/document/util/UtilsTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/test/java/org/apache/jackrabbit/oak/plugins/document/util/UtilsTest.java (revision 1642837) +++ src/test/java/org/apache/jackrabbit/oak/plugins/document/util/UtilsTest.java (revision ) @@ -24,6 +24,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; /** @@ -90,5 +91,19 @@ } time = System.currentTimeMillis() - time; System.out.println(time); + } + + @Test + public void max() { + Revision a = new Revision(42, 0, 1); + Revision b = new Revision(43, 0, 1); + assertSame(b, Utils.max(a, b)); + + Revision a1 = new Revision(42, 1, 1); + assertSame(a1, Utils.max(a, a1)); + + assertSame(a, Utils.max(a, null)); + assertSame(a, Utils.max(null, a)); + assertNull(Utils.max(null, null)); } } Index: src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgent.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgent.java (revision 1642837) +++ src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgent.java (revision ) @@ -155,15 +155,10 @@ if (currentLastRev != null) { knownLastRevs.put(doc.getPath(), currentLastRev); } - Revision lostLastRev = determineMissedLastRev(doc, clusterId); + Revision lastModifiedRev = determineLastModification(doc, clusterId); - //1. Update lastRev for this doc - if (lostLastRev != null) { - unsaved.put(doc.getPath(), lostLastRev); - } + Revision lastRevForParents = Utils.max(lastModifiedRev, currentLastRev); - Revision lastRevForParents = lostLastRev != null ? lostLastRev : currentLastRev; - //If both currentLastRev and lostLastRev are null it means //that no change is done by suspect cluster on this document //so nothing needs to be updated. Probably it was only changed by @@ -257,21 +252,17 @@ } /** - * Determines the last revision value which needs to set for given clusterId - * on the passed document. If the last rev entries are consisted + * Determines the last committed modification to the given document by + * a {@code clusterId}. * - * @param doc NodeDocument where lastRev entries needs to be fixed - * @param clusterId clusterId for which lastRev has to be checked - * @return lastRev which needs to be updated. null if no - * updated is required i.e. lastRev entries are valid + * @param doc a document. + * @param clusterId clusterId for which the last committed modification is + * looked up. + * @return the commit revision of the last modification by {@code clusterId} + * to the given document. */ @CheckForNull - private Revision determineMissedLastRev(NodeDocument doc, int clusterId) { - Revision currentLastRev = doc.getLastRev().get(clusterId); - if (currentLastRev == null) { - currentLastRev = new Revision(0, 0, clusterId); - } - + private Revision determineLastModification(NodeDocument doc, int clusterId) { ClusterPredicate cp = new ClusterPredicate(clusterId); // Merge sort the revs for which changes have been made @@ -285,22 +276,12 @@ StableRevisionComparator.REVERSE ); - // Look for latest valid revision > currentLastRev - // if found then lastRev needs to be fixed + Revision lastModified = null; + // Look for latest valid revision for (Revision rev : revs) { - if (rev.compareRevisionTime(currentLastRev) > 0) { - rev = doc.getCommitRevision(rev); - if (rev != null) { - return rev; + lastModified = Utils.max(lastModified, doc.getCommitRevision(rev)); - } + } - } else { - // No valid revision found > currentLastRev - // indicates that lastRev is valid for given clusterId - // and no further checks are required - break; - } - } - return null; + return lastModified; } /** Index: src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java (revision 1642837) +++ src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java (revision ) @@ -932,33 +932,15 @@ * @param rev the commit revision * @param path the path * @param isNew whether this is a new node - * @param pendingLastRev whether the node has a pending _lastRev to write - * @param isBranchCommit whether this is from a branch commit * @param added the list of added child nodes * @param removed the list of removed child nodes * @param changed the list of changed child nodes. * */ public void applyChanges(Revision rev, String path, - boolean isNew, boolean pendingLastRev, - boolean isBranchCommit, List added, + boolean isNew, List added, List removed, List changed, DiffCache.Entry cacheEntry) { - LastRevTracker tracker = createTracker(rev); - if (disableBranches) { - if (pendingLastRev) { - tracker.track(path); - } - } else { - if (isBranchCommit) { - Revision branchRev = rev.asBranchRevision(); - tracker = branches.getBranchCommit(branchRev); - } - if (isBranchCommit || pendingLastRev) { - // write back _lastRev with background thread - tracker.track(path); - } - } if (isNew) { DocumentNodeState.Children c = new DocumentNodeState.Children(); Set set = Sets.newTreeSet(); @@ -1295,6 +1277,28 @@ return writer.toString(); } + /** + * Creates a tracker for the given commit revision. + * + * @param r a commit revision. + * @param isBranchCommit whether this is a branch commit. + * @return a _lastRev tracker for the given commit revision. + */ + LastRevTracker createTracker(final @Nonnull Revision r, + final boolean isBranchCommit) { + if (isBranchCommit && !disableBranches) { + Revision branchRev = r.asBranchRevision(); + return branches.getBranchCommit(branchRev); + } else { + return new LastRevTracker() { + @Override + public void track(String path) { + unsavedLastRevisions.put(path, r); + } + }; + } + } + //------------------------< Observable >------------------------------------ @Override @@ -1601,21 +1605,6 @@ } //-----------------------------< internal >--------------------------------- - - /** - * Creates a tracker for the given commit revision. - * - * @param r a commit revision. - * @return a _lastRev tracker for the given commit revision. - */ - private LastRevTracker createTracker(final @Nonnull Revision r) { - return new LastRevTracker() { - @Override - public void track(String path) { - unsavedLastRevisions.put(path, r); - } - }; - } private static void diffProperties(DocumentNodeState from, DocumentNodeState to,