Thats simple and precise solution to a nasty problem. :) Chetan Mehrotra
On Wed, Oct 8, 2014 at 9:29 PM, <mreut...@apache.org> wrote: > Author: mreutegg > Date: Wed Oct 8 15:59:30 2014 > New Revision: 1630156 > > URL: http://svn.apache.org/r1630156 > Log: > OAK-2144: Intermittent Node not found at given revision with DocumentNodeStore > > Detect when _lastRevs are ambiguous and use readRevision instead. > Enable tests. > > Modified: > > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java > > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterRevisionComparisonTest.java > > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java > > Modified: > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java > URL: > http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java?rev=1630156&r1=1630155&r2=1630156&view=diff > ============================================================================== > --- > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java > (original) > +++ > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java > Wed Oct 8 15:59:30 2014 > @@ -823,7 +823,11 @@ public final class NodeDocument extends > // changes is the base of the branch > r = branchBase; > } > - if (isRevisionNewer(nodeStore, r, lastRevision)) { > + if (revisionAreAmbiguous(nodeStore, r, lastRevision)) { > + // _lastRev entries from multiple cluster nodes are ambiguous > + // use readRevision to make sure read is consistent > + lastRevision = readRevision; > + } else if (isRevisionNewer(nodeStore, r, lastRevision)) { > lastRevision = r; > } > } > @@ -1201,6 +1205,34 @@ public final class NodeDocument extends > //----------------------------< internal > >---------------------------------- > > /** > + * Returns {@code true} if the two revisions are ambiguous. That is, they > + * are from different cluster nodes and the comparison of the two > revision > + * depends on the seen at revision and is different when just comparing > the > + * timestamps of the revisions. > + * > + * @param context the revision context. > + * @param r1 the first revision. > + * @param r2 the second revision. > + * @return {@code true} if ambiguous, {@code false} otherwise. > + */ > + static boolean revisionAreAmbiguous(@Nonnull RevisionContext context, > + @Nonnull Revision r1, > + @Nonnull Revision r2) { > + if (r1.getClusterId() == r2.getClusterId()) { > + return false; > + } > + int c1 = context.getRevisionComparator().compare(r1, r2); > + int c2 = r1.compareRevisionTimeThenClusterId(r2); > + if (c1 == 0) { > + return c2 == 0; > + } else if (c1 < 0) { > + return c2 >= 0; > + } else { > + return c2 <= 0; > + } > + } > + > + /** > * Returns the commit root document for the given revision. This may > either > * be this document or another one. > * > > Modified: > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterRevisionComparisonTest.java > URL: > http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterRevisionComparisonTest.java?rev=1630156&r1=1630155&r2=1630156&view=diff > ============================================================================== > --- > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterRevisionComparisonTest.java > (original) > +++ > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterRevisionComparisonTest.java > Wed Oct 8 15:59:30 2014 > @@ -36,7 +36,6 @@ import org.apache.jackrabbit.oak.spi.sta > import org.apache.jackrabbit.oak.stats.Clock; > import org.junit.After; > import org.junit.Before; > -import org.junit.Ignore; > import org.junit.Test; > > import static org.junit.Assert.assertTrue; > @@ -52,7 +51,6 @@ public class ClusterRevisionComparisonTe > Revision.setClock(clock); > } > > - @Ignore("OAK-2144") //FIX ME OAK-2144 > @Test > public void revisionComparisonMultipleClusterNode() throws Exception{ > DocumentNodeStore c1 = createNS(1); > @@ -109,7 +107,6 @@ public class ClusterRevisionComparisonTe > c1ns1.compareAgainstBaseState(c1ns2, new ClusterTest.TrackingDiff()); > } > > - @Ignore("OAK-2144") > @Test > public void revisionComparisonTwoClusterNodes() throws Exception { > DocumentNodeStore c1 = createNS(1); > @@ -139,8 +136,8 @@ public class ClusterRevisionComparisonTe > > // 6. Purge revision comparator. Also purge entries from nodeCache > // such that later reads at rT1-C2 triggers read from underlying > DocumentStore > - c1.invalidateNodeCache("/a/c1" , > ((DocumentNodeState)c1ns1.getChildNode("a")).getLastRevision()); > - c1.invalidateNodeCache("/a/c2" , > ((DocumentNodeState)c1ns1.getChildNode("a")).getLastRevision()); > + c1.invalidateNodeCache("/a/c1" , > ((DocumentNodeState)a).getLastRevision()); > + c1.invalidateNodeCache("/a/c2" , > ((DocumentNodeState)a).getLastRevision()); > > //Revision comparator purge by moving in future > clock.waitUntil(clock.getTime() + > DocumentNodeStore.REMEMBER_REVISION_ORDER_MILLIS * 2); > @@ -148,6 +145,19 @@ public class ClusterRevisionComparisonTe > > assertTrue("/a/c1 disappeared", a.hasChildNode("c1")); > assertTrue("/a/c2 disappeared", a.hasChildNode("c2")); > + > + // read again starting at root node with a invalidated cache > + // and purged revision comparator > + c1ns1 = c1.getRoot(); > + c1.invalidateNodeCache("/", c1ns1.getRevision()); > + c1ns1 = c1.getRoot(); > + c1.invalidateNodeCache("/a", c1ns1.getLastRevision()); > + assertTrue("/a missing", c1ns1.hasChildNode("a")); > + a = c1ns1.getChildNode("a"); > + c1.invalidateNodeCache("/a/c1", > ((DocumentNodeState)a).getLastRevision()); > + c1.invalidateNodeCache("/a/c2", > ((DocumentNodeState)a).getLastRevision()); > + assertTrue("/a/c1 disappeared", a.hasChildNode("c1")); > + assertTrue("/a/c2 disappeared", a.hasChildNode("c2")); > } > > @After > > Modified: > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java > URL: > http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java?rev=1630156&r1=1630155&r2=1630156&view=diff > ============================================================================== > --- > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java > (original) > +++ > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java > Wed Oct 8 15:59:30 2014 > @@ -16,10 +16,17 @@ > */ > package org.apache.jackrabbit.oak.plugins.document; > > +import java.util.Comparator; > + > import org.apache.jackrabbit.oak.plugins.document.memory.MemoryDocumentStore; > import org.apache.jackrabbit.oak.plugins.document.util.Utils; > import org.junit.Test; > > +import static > org.apache.jackrabbit.oak.plugins.document.NodeDocument.revisionAreAmbiguous; > +import static > org.apache.jackrabbit.oak.plugins.document.Revision.RevisionComparator; > +import static org.junit.Assert.assertFalse; > +import static org.junit.Assert.assertTrue; > + > /** > * Tests for {@link NodeDocument}. > */ > @@ -40,4 +47,41 @@ public class NodeDocumentTest { > UpdateUtils.applyChanges(doc, op, StableRevisionComparator.INSTANCE); > doc.split(DummyRevisionContext.INSTANCE); > } > + > + @Test > + public void ambiguousRevisions() { > + // revisions from same cluster node are not ambiguous > + RevisionContext context = DummyRevisionContext.INSTANCE; > + Revision r1 = new Revision(1, 0, 1); > + Revision r2 = new Revision(2, 0, 1); > + assertFalse(revisionAreAmbiguous(context, r1, r1)); > + assertFalse(revisionAreAmbiguous(context, r1, r2)); > + assertFalse(revisionAreAmbiguous(context, r2, r1)); > + > + // revisions from different cluster nodes are not ambiguous > + // if seen with stable revision comparator > + r1 = new Revision(1, 0, 2); > + r2 = new Revision(2, 0, 1); > + assertFalse(revisionAreAmbiguous(context, r1, r1)); > + assertFalse(revisionAreAmbiguous(context, r1, r2)); > + assertFalse(revisionAreAmbiguous(context, r2, r1)); > + > + // now use a revision comparator with seen-at support > + final RevisionComparator comparator = new RevisionComparator(1); > + context = new DummyRevisionContext() { > + @Override > + public Comparator<Revision> getRevisionComparator() { > + return comparator; > + } > + }; > + r1 = new Revision(1, 0, 2); > + r2 = new Revision(2, 0, 1); > + // add revision to comparator in reverse time order > + comparator.add(r2, new Revision(2, 0, 0)); > + comparator.add(r1, new Revision(3, 0, 0)); // r1 seen after r2 > + assertFalse(revisionAreAmbiguous(context, r1, r1)); > + assertFalse(revisionAreAmbiguous(context, r2, r2)); > + assertTrue(revisionAreAmbiguous(context, r1, r2)); > + assertTrue(revisionAreAmbiguous(context, r2, r1)); > + } > } > >
