Author: mreutegg
Date: Wed Jul 8 12:12:32 2015
New Revision: 1689853
URL: http://svn.apache.org/r1689853
Log:
OAK-3081: SplitOperations may undo committed changes
Merged revisions 1689810,1689828,1689833 from trunk
Modified:
jackrabbit/oak/branches/1.2/ (props changed)
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/SplitOperations.java
jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentSplitTest.java
jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
Propchange: jackrabbit/oak/branches/1.2/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Wed Jul 8 12:12:32 2015
@@ -1,3 +1,3 @@
/jackrabbit/oak/branches/1.0:1665962
-/jackrabbit/oak/trunk:1672350,1672468,1672537,1672603,1672642,1672644,1672834-1672835,1673351,1673410,1673414-1673415,1673436,1673644,1673662-1673664,1673669,1673695,1673738,1673787,1673791,1674046,1674065,1674075,1674107,1674228,1674780,1674880,1675054-1675055,1675319,1675332,1675354,1675357,1675382,1675555,1675566,1675593,1676198,1676237,1676407,1676458,1676539,1676670,1676693,1676703,1676725,1677579,1677581,1677609,1677611,1677774,1677788,1677797,1677804,1677806,1677939,1677991,1678023,1678095-1678096,1678171,1678173,1678211,1678323,1678758,1678938,1678954,1679144,1679165,1679191,1679232,1679235,1679503,1679958,1679961,1680182,1680222,1680232,1680236,1680461,1680633,1680643,1680747,1680805-1680806,1680903,1681282,1681767,1681918,1682042,1682218,1682235,1682437,1682494,1682555,1682855,1682904,1683059,1683089,1683213,1683249,1683259,1683278,1683323,1683687,1683700,1684174-1684175,1684186,1684376,1684442,1684561,1684570,1684601,1684618,1684820,1684868,1685023,1685370,1685552,1685589
-1685590,1685840,1685964,1685977,1685989,1685999,1686023,1686032,1686097,1686162,1686229,1686234,1686253,1686414,1686780,1686854,1686857,1686971,1687053-1687055,1687175,1687198,1687220,1687239-1687240,1687301,1687441,1687553,1688089-1688090,1688172,1688179,1688349,1688421,1688436,1688453,1688616,1688622,1688636,1688817,1689003,1689577,1689581,1689623
+/jackrabbit/oak/trunk:1672350,1672468,1672537,1672603,1672642,1672644,1672834-1672835,1673351,1673410,1673414-1673415,1673436,1673644,1673662-1673664,1673669,1673695,1673738,1673787,1673791,1674046,1674065,1674075,1674107,1674228,1674780,1674880,1675054-1675055,1675319,1675332,1675354,1675357,1675382,1675555,1675566,1675593,1676198,1676237,1676407,1676458,1676539,1676670,1676693,1676703,1676725,1677579,1677581,1677609,1677611,1677774,1677788,1677797,1677804,1677806,1677939,1677991,1678023,1678095-1678096,1678171,1678173,1678211,1678323,1678758,1678938,1678954,1679144,1679165,1679191,1679232,1679235,1679503,1679958,1679961,1680182,1680222,1680232,1680236,1680461,1680633,1680643,1680747,1680805-1680806,1680903,1681282,1681767,1681918,1682042,1682218,1682235,1682437,1682494,1682555,1682855,1682904,1683059,1683089,1683213,1683249,1683259,1683278,1683323,1683687,1683700,1684174-1684175,1684186,1684376,1684442,1684561,1684570,1684601,1684618,1684820,1684868,1685023,1685370,1685552,1685589
-1685590,1685840,1685964,1685977,1685989,1685999,1686023,1686032,1686097,1686162,1686229,1686234,1686253,1686414,1686780,1686854,1686857,1686971,1687053-1687055,1687175,1687198,1687220,1687239-1687240,1687301,1687441,1687553,1688089-1688090,1688172,1688179,1688349,1688421,1688436,1688453,1688616,1688622,1688636,1688817,1689003,1689577,1689581,1689623,1689810,1689828,1689833
/jackrabbit/trunk:1345480
Modified:
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java?rev=1689853&r1=1689852&r2=1689853&view=diff
==============================================================================
---
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
(original)
+++
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
Wed Jul 8 12:12:32 2015
@@ -1960,13 +1960,14 @@ public final class DocumentNodeStore
}
private void backgroundSplit() {
+ Revision head = getHeadRevision();
for (Iterator<String> it = splitCandidates.keySet().iterator();
it.hasNext();) {
String id = it.next();
NodeDocument doc = store.find(Collection.NODES, id);
if (doc == null) {
continue;
}
- for (UpdateOp op : doc.split(this)) {
+ for (UpdateOp op : doc.split(this, head)) {
NodeDocument before = store.createOrUpdate(Collection.NODES,
op);
if (before != null) {
if (LOG.isDebugEnabled()) {
@@ -1984,6 +1985,11 @@ public final class DocumentNodeStore
}
}
+ @Nonnull
+ Set<String> getSplitCandidates() {
+ return Collections.unmodifiableSet(splitCandidates.keySet());
+ }
+
BackgroundWriteStats backgroundWrite() {
return unsavedLastRevisions.persist(this, new
UnsavedModifications.Snapshot() {
@Override
Modified:
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java?rev=1689853&r1=1689852&r2=1689853&view=diff
==============================================================================
---
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java
(original)
+++
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java
Wed Jul 8 12:12:32 2015
@@ -1040,14 +1040,21 @@ public final class NodeDocument extends
/**
* Returns update operations to split this document. The implementation may
- * decide to not return any operations if no splitting is required.
+ * decide to not return any operations if no splitting is required. A
caller
+ * must explicitly pass a head revision even though it is available through
+ * the {@link RevisionContext}. The given head revision must reflect a head
+ * state before {@code doc} was retrieved from the document store. This is
+ * important in order to maintain consistency. See OAK-3081 for details.
*
* @param context the revision context.
+ * @param head the head revision before this document was retrieved from
+ * the document store.
* @return the split operations.
*/
@Nonnull
- public Iterable<UpdateOp> split(@Nonnull RevisionContext context) {
- return SplitOperations.forDocument(this, context);
+ public Iterable<UpdateOp> split(@Nonnull RevisionContext context,
+ @Nonnull Revision head) {
+ return SplitOperations.forDocument(this, context, head);
}
/**
Modified:
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/SplitOperations.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/SplitOperations.java?rev=1689853&r1=1689852&r2=1689853&view=diff
==============================================================================
---
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/SplitOperations.java
(original)
+++
jackrabbit/oak/branches/1.2/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/SplitOperations.java
Wed Jul 8 12:12:32 2015
@@ -70,6 +70,7 @@ class SplitOperations {
private final NodeDocument doc;
private final String path;
private final String id;
+ private final Revision headRevision;
private final RevisionContext context;
private Revision high;
private Revision low;
@@ -84,19 +85,27 @@ class SplitOperations {
private UpdateOp main;
private SplitOperations(@Nonnull NodeDocument doc,
- @Nonnull RevisionContext context) {
+ @Nonnull RevisionContext context,
+ @Nonnull Revision headRevision) {
this.doc = checkNotNull(doc);
this.context = checkNotNull(context);
this.path = doc.getPath();
this.id = doc.getId();
+ this.headRevision = checkNotNull(headRevision);
}
/**
* Creates a list of update operations in case the given document requires
- * a split.
+ * a split. A caller must explicitly pass a head revision even though it
+ * is available through the {@link RevisionContext}. The given head
revision
+ * must reflect a head state before {@code doc} was retrieved from the
+ * document store. This is important in order to maintain consistency.
+ * See OAK-3081 for details.
*
* @param doc a main document.
* @param context the revision context.
+ * @param headRevision the head revision before the document was retrieved
+ * from the document store.
* @return list of update operations. An empty list indicates the document
* does not require a split.
* @throws IllegalArgumentException if the given document is a split
@@ -104,12 +113,13 @@ class SplitOperations {
*/
@Nonnull
static List<UpdateOp> forDocument(@Nonnull NodeDocument doc,
- @Nonnull RevisionContext context) {
+ @Nonnull RevisionContext context,
+ @Nonnull Revision headRevision) {
if (doc.isSplitDocument()) {
throw new IllegalArgumentException(
"Not a main document: " + doc.getId());
}
- return new SplitOperations(doc, context).create();
+ return new SplitOperations(doc, context, headRevision).create();
}
@@ -392,9 +402,10 @@ class SplitOperations {
}
private boolean isGarbage(Revision rev) {
- Revision head = context.getHeadRevision();
Comparator<Revision> comp = context.getRevisionComparator();
- if (comp.compare(head, rev) <= 0) {
+ // use headRevision as passed in the constructor instead
+ // of the head revision from the RevisionContext. see OAK-3081
+ if (comp.compare(headRevision, rev) <= 0) {
// this may be an in-progress commit
return false;
}
Modified:
jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentSplitTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentSplitTest.java?rev=1689853&r1=1689852&r2=1689853&view=diff
==============================================================================
---
jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentSplitTest.java
(original)
+++
jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentSplitTest.java
Wed Jul 8 12:12:32 2015
@@ -18,6 +18,7 @@ package org.apache.jackrabbit.oak.plugin
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Comparator;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
@@ -25,12 +26,16 @@ import java.util.Random;
import java.util.Set;
import java.util.TreeSet;
+import javax.annotation.Nonnull;
+
import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterators;
import org.apache.jackrabbit.oak.api.CommitFailedException;
import org.apache.jackrabbit.oak.commons.PathUtils;
+import org.apache.jackrabbit.oak.plugins.document.UpdateOp.Key;
+import org.apache.jackrabbit.oak.plugins.document.UpdateOp.Operation;
import org.apache.jackrabbit.oak.spi.blob.MemoryBlobStore;
import org.apache.jackrabbit.oak.plugins.document.memory.MemoryDocumentStore;
import org.apache.jackrabbit.oak.plugins.document.util.Utils;
@@ -531,7 +536,8 @@ public class DocumentSplitTest extends B
doc = store.find(NODES, id);
assertNotNull(doc);
- List<UpdateOp> splitOps =
Lists.newArrayList(doc.split(mk.getNodeStore()));
+ List<UpdateOp> splitOps = Lists.newArrayList(
+ doc.split(mk.getNodeStore(),
mk.getNodeStore().getHeadRevision()));
assertEquals(2, splitOps.size());
// first update op is for the new intermediate doc
op = splitOps.get(0);
@@ -540,7 +546,7 @@ public class DocumentSplitTest extends B
// second update op is for the main document
op = splitOps.get(1);
assertEquals(id, op.getId());
- for (Map.Entry<UpdateOp.Key, UpdateOp.Operation> entry :
op.getChanges().entrySet()) {
+ for (Map.Entry<Key, Operation> entry : op.getChanges().entrySet()) {
Revision r = entry.getKey().getRevision();
assertNotNull(r);
assertEquals(clusterId, r.getClusterId());
@@ -586,7 +592,8 @@ public class DocumentSplitTest extends B
// must split document and create a previous document starting at
// the second most recent revision
- List<UpdateOp> splitOps =
Lists.newArrayList(doc.split(mk.getNodeStore()));
+ List<UpdateOp> splitOps = Lists.newArrayList(
+ doc.split(mk.getNodeStore(),
mk.getNodeStore().getHeadRevision()));
assertEquals(2, splitOps.size());
String prevId = Utils.getPreviousIdFor("/test", revs.get(revs.size() -
2), 0);
assertEquals(prevId, splitOps.get(0).getId());
@@ -662,7 +669,8 @@ public class DocumentSplitTest extends B
NodeDocument doc = new NodeDocument(mk.getDocumentStore());
doc.put(NodeDocument.ID, Utils.getIdFromPath("/test"));
doc.put(NodeDocument.SD_TYPE, NodeDocument.SplitDocType.DEFAULT.type);
- SplitOperations.forDocument(doc, DummyRevisionContext.INSTANCE);
+ Revision head = mk.getNodeStore().getHeadRevision();
+ SplitOperations.forDocument(doc, DummyRevisionContext.INSTANCE, head);
}
@Test
@@ -732,6 +740,120 @@ public class DocumentSplitTest extends B
assertTrue(doc.getLocalCommitRoot().size() < NUM_REVS_THRESHOLD);
}
+ // OAK-3081
+ @Test
+ public void removeGarbage() throws Exception {
+ final DocumentStore store = mk.getDocumentStore();
+ final DocumentNodeStore ns = mk.getNodeStore();
+ final List<Exception> exceptions = Lists.newArrayList();
+ final List<Revision> revisions = Lists.newArrayList();
+
+ Thread t = new Thread(new Runnable() {
+ @Override
+ public void run() {
+ try {
+ for (int i = 0; i < 200; i++) {
+ NodeBuilder builder = ns.getRoot().builder();
+
builder.child("foo").child("node").child("node").child("node").child("node");
+
builder.child("bar").child("node").child("node").child("node").child("node");
+ merge(ns, builder);
+ revisions.add(ns.getHeadRevision());
+
+ builder = ns.getRoot().builder();
+ builder.child("foo").child("node").remove();
+ builder.child("bar").child("node").remove();
+ merge(ns, builder);
+ revisions.add(ns.getHeadRevision());
+ }
+ } catch (CommitFailedException e) {
+ exceptions.add(e);
+ }
+ }
+ });
+ t.start();
+
+ // Use a revision context, which wraps the DocumentNodeStore and
+ // randomly delays calls to get the head revision
+ RevisionContext rc = new TestRevisionContext(ns);
+ while (t.isAlive()) {
+ for (String id : ns.getSplitCandidates()) {
+ Revision head = ns.getHeadRevision();
+ NodeDocument doc = store.find(NODES, id);
+ List<UpdateOp> ops = SplitOperations.forDocument(doc, rc,
head);
+ Set<Revision> removed = Sets.newHashSet();
+ Set<Revision> added = Sets.newHashSet();
+ for (UpdateOp op : ops) {
+ for (Map.Entry<Key, Operation> e :
op.getChanges().entrySet()) {
+ if (!"_deleted".equals(e.getKey().getName())) {
+ continue;
+ }
+ Revision r = e.getKey().getRevision();
+ if (e.getValue().type ==
Operation.Type.REMOVE_MAP_ENTRY) {
+ removed.add(r);
+ } else if (e.getValue().type ==
Operation.Type.SET_MAP_ENTRY) {
+ added.add(r);
+ }
+ }
+ }
+ removed.removeAll(added);
+ assertTrue("SplitOperations must not remove committed changes:
" + removed, removed.isEmpty());
+ }
+ // perform the actual cleanup
+ ns.runBackgroundOperations();
+ }
+
+ // check documents below /foo and /bar
+ // the _deleted map must contain all revisions
+ for (NodeDocument doc : Utils.getAllDocuments(store)) {
+ if (doc.isSplitDocument() || Utils.getDepthFromId(doc.getId()) <
2) {
+ continue;
+ }
+ Set<Revision> revs = Sets.newHashSet(revisions);
+ revs.removeAll(doc.getValueMap("_deleted").keySet());
+ assertTrue("Missing _deleted entries on " + doc.getId() + ": " +
revs, revs.isEmpty());
+ }
+ }
+
+ private static class TestRevisionContext implements RevisionContext {
+
+ private final RevisionContext rc;
+
+ TestRevisionContext(RevisionContext rc) {
+ this.rc = rc;
+ }
+
+ @Override
+ public UnmergedBranches getBranches() {
+ return rc.getBranches();
+ }
+
+ @Override
+ public UnsavedModifications getPendingModifications() {
+ return rc.getPendingModifications();
+ }
+
+ @Override
+ public Comparator<Revision> getRevisionComparator() {
+ return rc.getRevisionComparator();
+ }
+
+ @Override
+ public int getClusterId() {
+ return rc.getClusterId();
+ }
+
+ @Nonnull
+ @Override
+ public Revision getHeadRevision() {
+ try {
+ Thread.sleep((long) (Math.random() * 100));
+ } catch (InterruptedException e) {
+ // ignore
+ }
+ return rc.getHeadRevision();
+ }
+ }
+
private static NodeState merge(NodeStore store, NodeBuilder root)
throws CommitFailedException {
return store.merge(root, EmptyHook.INSTANCE, CommitInfo.EMPTY);
Modified:
jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java?rev=1689853&r1=1689852&r2=1689853&view=diff
==============================================================================
---
jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
(original)
+++
jackrabbit/oak/branches/1.2/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
Wed Jul 8 12:12:32 2015
@@ -45,7 +45,8 @@ public class NodeDocumentTest {
NodeDocument.addCollision(op, r);
}
UpdateUtils.applyChanges(doc, op, StableRevisionComparator.INSTANCE);
- doc.split(DummyRevisionContext.INSTANCE);
+ Revision head = DummyRevisionContext.INSTANCE.getHeadRevision();
+ doc.split(DummyRevisionContext.INSTANCE, head);
}
@Test
