Hi Marcel, A short description of what was the fix would be very helpful for future reference! Chetan Mehrotra
On Tue, Sep 22, 2015 at 9:00 PM, <mreut...@apache.org> wrote: > Author: mreutegg > Date: Tue Sep 22 15:30:08 2015 > New Revision: 1704655 > > URL: http://svn.apache.org/viewvc?rev=1704655&view=rev > Log: > OAK-3433: Commit does not detect conflict when background read happens after > rebase > > Add yet another test, enable existing test and implement fix > > Modified: > > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java > > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgent.java > > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UnsavedModifications.java > > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java > > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/ClusterConflictTest.java > > Modified: > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java > URL: > http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java?rev=1704655&r1=1704654&r2=1704655&view=diff > ============================================================================== > --- > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java > (original) > +++ > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java > Tue Sep 22 15:30:08 2015 > @@ -2084,9 +2084,9 @@ public final class DocumentNodeStore > BackgroundWriteStats backgroundWrite() { > return unsavedLastRevisions.persist(this, new > UnsavedModifications.Snapshot() { > @Override > - public void acquiring() { > + public void acquiring(Revision mostRecent) { > if (store.create(JOURNAL, > - > singletonList(changes.asUpdateOp(getHeadRevision())))) { > + singletonList(changes.asUpdateOp(mostRecent)))) { > changes = JOURNAL.newDocument(getDocumentStore()); > } > } > > Modified: > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgent.java > URL: > http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgent.java?rev=1704655&r1=1704654&r2=1704655&view=diff > ============================================================================== > --- > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgent.java > (original) > +++ > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/LastRevRecoveryAgent.java > Tue Sep 22 15:30:08 2015 > @@ -235,7 +235,7 @@ public class LastRevRecoveryAgent { > unsaved.persist(nodeStore, new UnsavedModifications.Snapshot() { > > @Override > - public void acquiring() { > + public void acquiring(Revision mostRecent) { > if (lastRootRev == null) { > // this should never happen - when unsaved has no > changes > // that is reflected in the 'map' to be empty - in > that > > Modified: > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UnsavedModifications.java > URL: > http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UnsavedModifications.java?rev=1704655&r1=1704654&r2=1704655&view=diff > ============================================================================== > --- > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UnsavedModifications.java > (original) > +++ > jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UnsavedModifications.java > Tue Sep 22 15:30:08 2015 > @@ -159,7 +159,7 @@ class UnsavedModifications { > time = clock.getTime(); > Map<String, Revision> pending; > try { > - snapshot.acquiring(); > + snapshot.acquiring(getMostRecentRevision()); > pending = Maps.newTreeMap(PathComparator.INSTANCE); > pending.putAll(map); > } finally { > @@ -234,14 +234,26 @@ class UnsavedModifications { > return map.toString(); > } > > + private Revision getMostRecentRevision() { > + // use revision of root document > + Revision rev = map.get("/"); > + // otherwise find most recent > + if (rev == null) { > + for (Revision r : map.values()) { > + rev = Utils.max(rev, r); > + } > + } > + return rev; > + } > + > public interface Snapshot { > > Snapshot IGNORE = new Snapshot() { > @Override > - public void acquiring() { > + public void acquiring(Revision mostRecent) { > } > }; > > - void acquiring(); > + void acquiring(Revision mostRecent); > } > } > > Modified: > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java > URL: > http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java?rev=1704655&r1=1704654&r2=1704655&view=diff > ============================================================================== > --- > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java > (original) > +++ > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java > Tue Sep 22 15:30:08 2015 > @@ -444,6 +444,31 @@ public class JournalTest extends Abstrac > } > } > > + // OAK-3433 > + @Test > + public void journalEntryKey() throws Exception { > + DocumentNodeStore ns1 = createMK(1, 0).getNodeStore(); > + DocumentNodeStore ns2 = createMK(2, 0).getNodeStore(); > + > + NodeBuilder b1 = ns1.getRoot().builder(); > + b1.child("foo"); > + ns1.merge(b1, EmptyHook.INSTANCE, CommitInfo.EMPTY); > + ns1.runBackgroundOperations(); > + > + NodeBuilder b2 = ns2.getRoot().builder(); > + b2.child("bar"); > + ns2.merge(b2, EmptyHook.INSTANCE, CommitInfo.EMPTY); > + Revision h2 = ns2.getHeadRevision(); > + > + ns2.runBackgroundReadOperations(); > + > + ns2.runBackgroundOperations(); > + > + String id = JournalEntry.asId(h2); > + assertTrue("Background update did not create a journal entry with id > " + id, > + ns1.getDocumentStore().find(Collection.JOURNAL, id) != null); > + } > + > private DocumentMK createMK(int clusterId, int asyncDelay) { > if (ds == null) { > ds = new MemoryDocumentStore(); > > Modified: > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/ClusterConflictTest.java > URL: > http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/ClusterConflictTest.java?rev=1704655&r1=1704654&r2=1704655&view=diff > ============================================================================== > --- > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/ClusterConflictTest.java > (original) > +++ > jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/ClusterConflictTest.java > Tue Sep 22 15:30:08 2015 > @@ -32,10 +32,8 @@ import org.apache.jackrabbit.oak.spi.com > import org.apache.jackrabbit.oak.spi.state.NodeBuilder; > import org.apache.jackrabbit.oak.spi.state.NodeState; > import org.apache.jackrabbit.oak.spi.state.NodeStore; > -import org.junit.Ignore; > import org.junit.Test; > > -import static org.junit.Assert.assertFalse; > import static org.junit.Assert.assertTrue; > import static org.junit.Assert.fail; > > @@ -61,7 +59,6 @@ public class ClusterConflictTest extends > } > > // OAK-3433 > - @Ignore > @Test > public void mergeRetryWhileBackgroundRead() throws Exception { > DocumentNodeStore ns1 = mk.getNodeStore(); > @@ -100,7 +97,7 @@ public class ClusterConflictTest extends > // because /a/b/c is seen as changed in the future > // without the fix for OAK-3433: > // the second merge attempt succeeds because now the > - // /a/b/c change is visible and happened before the commit > + // /a/b/c change revision is visible and happened before the commit > // revision but before the base revision > b2 = ns2.getRoot().builder(); > b2.child("z").setProperty("q", "v"); > @@ -115,13 +112,18 @@ public class ClusterConflictTest extends > runBackgroundRead(ns2); > > NodeBuilder builder = after.builder(); > - builder.child("a").child("b").child("c").child("bar"); > + if > (builder.getChildNode("a").getChildNode("b").hasChildNode("c")) { > + > builder.child("a").child("b").child("c").child("bar"); > + } else { > + throw new CommitFailedException( > + CommitFailedException.OAK, 0, > + "/a/b/c does not exist anymore"); > + } > return builder.getNodeState(); > } > }, CommitInfo.EMPTY); > fail("Merge must fail with CommitFailedException"); > } catch (CommitFailedException e) { > - e.printStackTrace(); > // expected > } > } > >