You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by mr...@apache.org on 2013/05/29 15:38:23 UTC
svn commit: r1487481 - in /jackrabbit/oak/trunk/oak-mongomk/src:
main/java/org/apache/jackrabbit/mongomk/
main/java/org/apache/jackrabbit/mongomk/util/
test/java/org/apache/jackrabbit/mongomk/
Author: mreutegg
Date: Wed May 29 13:38:23 2013
New Revision: 1487481
URL: http://svn.apache.org/r1487481
Log:
OAK-846: Branch conflicts not detected by MongoMK
Modified:
jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Collision.java
jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/CollisionHandler.java
jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Commit.java
jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/MongoMK.java
jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/util/Utils.java
jackrabbit/oak/trunk/oak-mongomk/src/test/java/org/apache/jackrabbit/mongomk/ConflictTest.java
Modified: jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Collision.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Collision.java?rev=1487481&r1=1487480&r2=1487481&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Collision.java (original)
+++ jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Collision.java Wed May 29 13:38:23 2013
@@ -20,6 +20,8 @@ import java.util.Map;
import javax.annotation.Nonnull;
+import org.apache.jackrabbit.mk.api.MicroKernelException;
+import org.apache.jackrabbit.mongomk.DocumentStore.Collection;
import org.apache.jackrabbit.mongomk.util.Utils;
import org.apache.jackrabbit.oak.commons.PathUtils;
import org.slf4j.Logger;
@@ -29,7 +31,16 @@ import static com.google.common.base.Pre
/**
* A <code>Collision</code> happens when a commit modifies a node, which was
- * also modified in a branch commit, but the branch commit is not yet merged.
+ * also modified in another branch not visible to the current session. This
+ * includes the following situations:
+ * <ul>
+ * <li>Our commit goes to trunk and another session committed to a branch
+ * not yet merged back.</li>
+ * <li>Our commit goes to a branch and another session committed to trunk
+ * or some other branch.</li>
+ * </ul>
+ * Other collisions like concurrent commits to trunk are handled earlier and
+ * do not require collision marking. See {@link Commit#createOrUpdateNode()}.
*/
class Collision {
@@ -50,47 +61,115 @@ class Collision {
this.ourRev = checkNotNull(ourRev).toString();
}
- boolean mark(DocumentStore store) {
+ /**
+ * Marks the collision in the document store. Either our or their
+ * revision is annotated with a collision marker. Their revision is
+ * marked if it is not yet committed, otherwise our revision is marked.
+ *
+ * @param store the document store.
+ * @throws MicroKernelException if the mark operation fails.
+ */
+ void mark(DocumentStore store) throws MicroKernelException {
+ // first try to mark their revision
if (markCommitRoot(document, theirRev, store)) {
- return true;
+ return;
}
+ // their commit wins, we have to mark ourRev
+ Map<String, Object> newDoc = Utils.newMap();
+ Utils.deepCopyMap(document, newDoc);
+ MemoryDocumentStore.applyChanges(newDoc, ourOp);
+ if (!markCommitRoot(newDoc, ourRev, store)) {
+ throw new MicroKernelException("Unable to annotate our revision "
+ + "with collision marker. Our revision: " + ourRev
+ + ", document:\n" + Utils.formatDocument(newDoc));
+ }
+ }
+
+ /**
+ * Marks the commit root of the change to the given <code>document</code> in
+ * <code>revision</code>.
+ *
+ * @param document the MongoDB document.
+ * @param revision the revision of the commit to annotated with a collision
+ * marker.
+ * @param store the document store.
+ * @return <code>true</code> if the commit for the given revision was marked
+ * successfully; <code>false</code> otherwise.
+ */
+ private static boolean markCommitRoot(@Nonnull Map<String, Object> document,
+ @Nonnull String revision,
+ @Nonnull DocumentStore store) {
+ String p = Utils.getPathFromId((String) document.get(UpdateOp.ID));
+ String commitRootPath = null;
+ // first check if we can mark the commit with the given revision
@SuppressWarnings("unchecked")
Map<String, String> revisions = (Map<String, String>) document.get(UpdateOp.REVISIONS);
- if (revisions.containsKey(theirRev)) {
- String value = revisions.get(theirRev);
+ if (revisions != null && revisions.containsKey(revision)) {
+ String value = revisions.get(revision);
if ("true".equals(value)) {
- // their commit wins, we have to mark ourRev
- Map<String, Object> newDoc = Utils.newMap();
- Utils.deepCopyMap(document, newDoc);
- MemoryDocumentStore.applyChanges(newDoc, ourOp);
- if (markCommitRoot(newDoc, ourRev, store)) {
- return true;
+ // already committed
+ return false;
+ } else {
+ // node is also commit root, but not yet committed
+ // i.e. a branch commit, which is not yet merged
+ commitRootPath = p;
+ }
+ } else {
+ // next look at commit root
+ @SuppressWarnings("unchecked")
+ Map<String, Integer> commitRoots = (Map<String, Integer>) document.get(UpdateOp.COMMIT_ROOT);
+ if (commitRoots != null) {
+ Integer depth = commitRoots.get(revision);
+ if (depth != null) {
+ commitRootPath = PathUtils.getAncestorPath(p, PathUtils.getDepth(p) - depth);
+ } else {
+ throwNoCommitRootException(revision, document);
}
+ } else {
+ throwNoCommitRootException(revision, document);
}
}
+ // at this point we have a commitRootPath
+ UpdateOp op = new UpdateOp(commitRootPath,
+ Utils.getIdFromPath(commitRootPath), false);
+ document = store.find(Collection.NODES, op.getKey());
+ // check commit status of revision
+ if (isCommitted(revision, document)) {
+ return false;
+ }
+ op.setMapEntry(UpdateOp.COLLISIONS, revision, true);
+ document = store.createOrUpdate(DocumentStore.Collection.NODES, op);
+ // check again on old document right before our update was applied
+ if (isCommitted(revision, document)) {
+ return false;
+ }
+ // otherwise collision marker was set successfully
+ LOG.debug("Marked collision on: {} for {} ({})",
+ new Object[]{commitRootPath, p, revision});
return true;
}
-
- private static boolean markCommitRoot(@Nonnull Map<String, Object> document,
- @Nonnull String revision,
- @Nonnull DocumentStore store) {
+
+ private static void throwNoCommitRootException(@Nonnull String revision,
+ @Nonnull Map<String, Object> document)
+ throws MicroKernelException {
+ throw new MicroKernelException("No commit root for revision: "
+ + revision + ", document: " + Utils.formatDocument(document));
+ }
+
+ /**
+ * Returns <code>true</code> if the given <code>revision</code> is marked
+ * committed on the given <code>document</code>.
+ *
+ * @param revision the revision.
+ * @param document a MongoDB document.
+ * @return <code>true</code> if committed; <code>false</code> otherwise.
+ */
+ private static boolean isCommitted(String revision, Map<String, Object> document) {
@SuppressWarnings("unchecked")
- Map<String, Integer> commitRoots = (Map<String, Integer>) document.get(UpdateOp.COMMIT_ROOT);
- if (commitRoots != null) {
- Integer depth = commitRoots.get(revision);
- if (depth != null) {
- String p = Utils.getPathFromId((String) document.get(UpdateOp.ID));
- String commitRootPath = PathUtils.getAncestorPath(p, PathUtils.getDepth(p) - depth);
- UpdateOp op = new UpdateOp(commitRootPath,
- Utils.getIdFromPath(commitRootPath), false);
- op.setMapEntry(UpdateOp.COLLISIONS, revision, true);
- // TODO: detect concurrent commit of previously un-merged changes
- // TODO: check _commitRoot for revision is not 'true'
- store.createOrUpdate(DocumentStore.Collection.NODES, op);
- LOG.debug("Marked collision on: {} for {} ({})",
- new Object[]{commitRootPath, p, revision});
- return true;
- }
+ Map<String, String> revisions = (Map<String, String>) document.get(UpdateOp.REVISIONS);
+ if (revisions != null && revisions.containsKey(revision)) {
+ String value = revisions.get(revision);
+ return "true".equals(value);
}
return false;
}
Modified: jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/CollisionHandler.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/CollisionHandler.java?rev=1487481&r1=1487480&r2=1487481&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/CollisionHandler.java (original)
+++ jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/CollisionHandler.java Wed May 29 13:38:23 2013
@@ -23,16 +23,16 @@ abstract class CollisionHandler {
static final CollisionHandler DEFAULT = new CollisionHandler() {
@Override
- void uncommittedModification(Revision uncommitted) {
+ void concurrentModification(Revision other) {
// do nothing
}
};
/**
- * Callback for an uncommitted modification in {@link Revision}
- * <code>uncommitted</code>.
+ * Callback for an concurrent modification in {@link Revision}
+ * <code>other</code>.
*
- * @param uncommitted the uncommitted revision of the change.
+ * @param other the revision of the concurrent change.
*/
- abstract void uncommittedModification(Revision uncommitted);
+ abstract void concurrentModification(Revision other);
}
Modified: jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Commit.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Commit.java?rev=1487481&r1=1487480&r2=1487481&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Commit.java (original)
+++ jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/Commit.java Wed May 29 13:38:23 2013
@@ -268,11 +268,11 @@ public class Commit {
Revision newestRev = mk.getNewestRevision(map, revision,
new CollisionHandler() {
@Override
- void uncommittedModification(Revision uncommitted) {
+ void concurrentModification(Revision other) {
if (collisions.get() == null) {
collisions.set(new ArrayList<Revision>());
}
- collisions.get().add(uncommitted);
+ collisions.get().add(other);
}
});
String conflictMessage = null;
@@ -296,7 +296,7 @@ public class Commit {
}
if (conflictMessage != null) {
conflictMessage += ", before\n" + revision +
- "; document:\n" + map.toString().replaceAll(", _", ",\n_").replaceAll("}, ", "},\n") +
+ "; document:\n" + Utils.formatDocument(map) +
",\nrevision order:\n" + mk.getRevisionComparator();
throw new MicroKernelException(conflictMessage);
}
@@ -306,11 +306,7 @@ public class Commit {
if (collisions.get() != null && isConflicting(map, op)) {
for (Revision r : collisions.get()) {
// mark collisions on commit root
- Collision c = new Collision(map, r, op, revision);
- boolean success = c.mark(store);
- if (!success) {
- // TODO: fail this commit
- }
+ new Collision(map, r, op, revision).mark(store);
}
}
}
Modified: jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/MongoMK.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/MongoMK.java?rev=1487481&r1=1487480&r2=1487481&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/MongoMK.java (original)
+++ jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/MongoMK.java Wed May 29 13:38:23 2013
@@ -1117,7 +1117,7 @@ public class MongoMK implements MicroKer
*
* @param nodeMap the document
* @param changeRev the revision of the current change
- * @param handler the conflict handler, which is called for un-committed revisions
+ * @param handler the conflict handler, which is called for concurrent changes
* preceding <code>before</code>.
* @return the revision, or null if deleted
*/
@@ -1152,7 +1152,7 @@ public class MongoMK implements MicroKer
if (!propRev.equals(changeRev)) {
if (!isValidRevision(
propRev, changeRev, nodeMap, new HashSet<Revision>())) {
- handler.uncommittedModification(propRev);
+ handler.concurrentModification(propRev);
} else {
newestRev = propRev;
}
Modified: jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/util/Utils.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/util/Utils.java?rev=1487481&r1=1487480&r2=1487481&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/util/Utils.java (original)
+++ jackrabbit/oak/trunk/oak-mongomk/src/main/java/org/apache/jackrabbit/mongomk/util/Utils.java Wed May 29 13:38:23 2013
@@ -181,5 +181,15 @@ public class Utils {
target.put(e.getKey(), value);
}
}
+
+ /**
+ * Formats a MongoDB document for use in a log message.
+ *
+ * @param document the MongoDB document.
+ * @return
+ */
+ public static String formatDocument(Map<String, Object> document) {
+ return document.toString().replaceAll(", _", ",\n_").replaceAll("}, ", "},\n");
+ }
}
Modified: jackrabbit/oak/trunk/oak-mongomk/src/test/java/org/apache/jackrabbit/mongomk/ConflictTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-mongomk/src/test/java/org/apache/jackrabbit/mongomk/ConflictTest.java?rev=1487481&r1=1487480&r2=1487481&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-mongomk/src/test/java/org/apache/jackrabbit/mongomk/ConflictTest.java (original)
+++ jackrabbit/oak/trunk/oak-mongomk/src/test/java/org/apache/jackrabbit/mongomk/ConflictTest.java Wed May 29 13:38:23 2013
@@ -17,7 +17,6 @@
package org.apache.jackrabbit.mongomk;
import org.apache.jackrabbit.mk.api.MicroKernelException;
-import org.junit.Ignore;
import org.junit.Test;
import static org.junit.Assert.fail;
@@ -89,6 +88,26 @@ public class ConflictTest extends BaseMo
}
@Test
+ public void addExistingPropertyTwoBranches() {
+ String rev = mk.commit("/", "+\"foo\":{}", null, null);
+ String branchRev1 = mk.branch(rev);
+ branchRev1 = mk.commit("/foo", "^\"prop\":\"value\"", branchRev1, null);
+
+ String branchRev2 = mk.branch(rev);
+ // will mark conflict on branchRev1
+ branchRev2 = mk.commit("/foo", "^\"prop\":\"other\"", branchRev2, null);
+
+ mk.merge(branchRev2, null);
+
+ try {
+ mk.merge(branchRev1, null);
+ fail("Must fail with conflict for addExistingProperty");
+ } catch (MicroKernelException e) {
+ // expected
+ }
+ }
+
+ @Test
public void removeRemovedProperty() {
String rev = mk.commit("/", "+\"foo\":{\"prop\":\"value\"}", null, null);
mk.commit("/foo", "^\"prop\":null", rev, null);
@@ -149,6 +168,26 @@ public class ConflictTest extends BaseMo
}
@Test
+ public void removeRemovedPropertyTwoBranches() {
+ String rev = mk.commit("/", "+\"foo\":{\"prop\":\"value\"}", null, null);
+ String branchRev1 = mk.branch(rev);
+ branchRev1 = mk.commit("/foo", "^\"prop\":null", branchRev1, null);
+
+ String branchRev2 = mk.branch(rev);
+ // will mark collision on branchRev1
+ branchRev2 = mk.commit("/foo", "^\"prop\":null", branchRev2, null);
+
+ mk.merge(branchRev2, null);
+
+ try {
+ mk.merge(branchRev1, null);
+ fail("Must fail with conflict for removeRemovedProperty");
+ } catch (MicroKernelException e) {
+ // expected
+ }
+ }
+
+ @Test
public void removeChangedProperty() {
String rev = mk.commit("/", "+\"foo\":{\"prop\":\"value\"}", null, null);
mk.commit("/foo", "^\"prop\":\"bar\"", rev, null);
@@ -209,6 +248,25 @@ public class ConflictTest extends BaseMo
}
@Test
+ public void removeChangedPropertyTwoBranches() {
+ String rev = mk.commit("/", "+\"foo\":{\"prop\":\"value\"}", null, null);
+ String branchRev1 = mk.branch(rev);
+ branchRev1 = mk.commit("/foo", "^\"prop\":null", branchRev1, null);
+
+ String branchRev2 = mk.branch(rev);
+ // will mark conflict on branchRev1
+ branchRev2 = mk.commit("/foo", "^\"prop\":\"bar\"", branchRev2, null);
+ mk.merge(branchRev2, null);
+
+ try {
+ mk.merge(branchRev1, null);
+ fail("Must fail with conflict for removeChangedProperty");
+ } catch (MicroKernelException e) {
+ // expected
+ }
+ }
+
+ @Test
public void changeRemovedProperty() {
String rev = mk.commit("/", "+\"foo\":{\"prop\":\"value\"}", null, null);
mk.commit("/foo", "^\"prop\":null", rev, null);
@@ -269,6 +327,25 @@ public class ConflictTest extends BaseMo
}
@Test
+ public void changeRemovedPropertyTwoBranches() {
+ String rev = mk.commit("/", "+\"foo\":{\"prop\":\"value\"}", null, null);
+ String branchRev1 = mk.branch(rev);
+ branchRev1 = mk.commit("/foo", "^\"prop\":\"bar\"", branchRev1, null);
+
+ String branchRev2 = mk.branch(rev);
+ // will mark collision on branchRev1
+ branchRev2 = mk.commit("/foo", "^\"prop\":null", branchRev2, null);
+ mk.merge(branchRev2, null);
+
+ try {
+ mk.merge(branchRev1, null);
+ fail("Must fail with conflict for changeRemovedProperty");
+ } catch (MicroKernelException e) {
+ // expected
+ }
+ }
+
+ @Test
public void changeChangedProperty() {
String rev = mk.commit("/", "+\"foo\":{\"prop\":\"value\"}", null, null);
mk.commit("/foo", "^\"prop\":\"bar\"", rev, null);
@@ -328,7 +405,6 @@ public class ConflictTest extends BaseMo
}
}
- @Ignore("OAK-846")
@Test
public void changeChangedPropertyTwoBranches(){
String rev = mk.commit("/", "+\"foo\":{\"prop\":\"value\"}", null, null);
@@ -409,6 +485,26 @@ public class ConflictTest extends BaseMo
}
@Test
+ public void addExistingNodeTwoBranches() {
+ String rev = mk.commit("/", "+\"foo\":{}", null, null);
+ String branchRev1 = mk.branch(rev);
+ branchRev1 = mk.commit("/foo", "+\"bar\":{}", branchRev1, null);
+
+ String branchRev2 = mk.branch(rev);
+ // will mark conflict on branchRev1
+ branchRev2 = mk.commit("/foo", "+\"bar\":{}", branchRev2, null);
+
+ mk.merge(branchRev2, null);
+
+ try {
+ mk.merge(branchRev1, null);
+ fail("Must fail with conflict for addExistingNode");
+ } catch (MicroKernelException e) {
+ // expected
+ }
+ }
+
+ @Test
public void removeRemovedNode() {
String rev = mk.commit("/", "+\"foo\":{}", null, null);
mk.commit("/", "-\"foo\"", rev, null);
@@ -469,6 +565,25 @@ public class ConflictTest extends BaseMo
}
@Test
+ public void removeRemovedNodeTwoBranches() {
+ String rev = mk.commit("/", "+\"foo\":{}", null, null);
+ String branchRev1 = mk.branch(rev);
+ branchRev1 = mk.commit("/", "-\"foo\"", branchRev1, null);
+
+ String branchRev2 = mk.branch(rev);
+ // will mark collision on branchRev1
+ branchRev2 = mk.commit("/", "-\"foo\"", branchRev2, null);
+ mk.merge(branchRev2, null);
+
+ try {
+ mk.merge(branchRev1, null);
+ fail("Must fail with conflict for removeRemovedNode");
+ } catch (MicroKernelException e) {
+ // expected
+ }
+ }
+
+ @Test
public void removeChangedNode() {
String rev = mk.commit("/", "+\"foo\":{}", null, null);
mk.commit("/foo", "^\"prop\":\"value\"", rev, null);
@@ -529,6 +644,25 @@ public class ConflictTest extends BaseMo
}
@Test
+ public void removeChangedNodeTwoBranches() {
+ String rev = mk.commit("/", "+\"foo\":{}", null, null);
+ String branchRev1 = mk.branch(rev);
+ branchRev1 = mk.commit("/", "-\"foo\"", branchRev1, null);
+
+ String branchRev2 = mk.branch(rev);
+ // will mark collision on branchRev1
+ branchRev2 = mk.commit("/foo", "^\"prop\":\"value\"", branchRev2, null);
+ mk.merge(branchRev2, null);
+
+ try {
+ mk.merge(branchRev1, null);
+ fail("Must fail with conflict for removeChangedNode");
+ } catch (MicroKernelException e) {
+ // expected
+ }
+ }
+
+ @Test
public void changeRemovedNode() {
String rev = mk.commit("/", "+\"foo\":{}", null, null);
mk.commit("/", "-\"foo\"", rev, null);
@@ -589,6 +723,25 @@ public class ConflictTest extends BaseMo
}
@Test
+ public void changeRemovedNodeTwoBranches() {
+ String rev = mk.commit("/", "+\"foo\":{}", null, null);
+ String branchRev1 = mk.branch(rev);
+ branchRev1 = mk.commit("/foo", "^\"prop\":\"value\"", branchRev1, null);
+
+ String branchRev2 = mk.branch(rev);
+ // will mark collision on branchRev1
+ branchRev2 = mk.commit("/", "-\"foo\"", branchRev2, null);
+ mk.merge(branchRev2, null);
+
+ try {
+ mk.merge(branchRev1, null);
+ fail("Must fail with conflict for changeRemovedNode");
+ } catch (MicroKernelException e) {
+ // expected
+ }
+ }
+
+ @Test
public void nonConflictingChangeProperty() {
String rev = mk.commit("/", "+\"foo\":{\"prop1\":\"value\", \"prop2\":\"value\"}", null, null);
mk.commit("/foo", "^\"prop1\":\"bar\"", rev, null);