Index: src/main/java/org/apache/jackrabbit/mongomk/impl/action/FetchCommitsAction.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/action/FetchCommitsAction.java (revision 1430720) +++ src/main/java/org/apache/jackrabbit/mongomk/impl/action/FetchCommitsAction.java (working copy) @@ -85,18 +85,22 @@ * Sets the max number of entries that should be fetched. * * @param maxEntries The max number of entries. + * @return this action. */ - public void setMaxEntries(int maxEntries) { + public FetchCommitsAction setMaxEntries(int maxEntries) { this.maxEntries = maxEntries; + return this; } /** * Sets whether the branch commits are included in the query. * * @param includeBranchCommits Whether the branch commits are included. + * @return this action. */ - public void includeBranchCommits(boolean includeBranchCommits) { + public FetchCommitsAction includeBranchCommits(boolean includeBranchCommits) { this.includeBranchCommits = includeBranchCommits; + return this; } @Override Index: src/main/java/org/apache/jackrabbit/mongomk/impl/action/FetchNodesActionNew.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/action/FetchNodesActionNew.java (revision 1430720) +++ src/main/java/org/apache/jackrabbit/mongomk/impl/action/FetchNodesActionNew.java (working copy) @@ -203,14 +203,14 @@ // Assuming that revision ids are ordered and nodes are fetched in // sorted order, first check if the path is already in the map. if (nodes.containsKey(path)) { - LOG.debug("Converted nodes @{} with path {} was not put into map" - + " because a newer version is available", revisionId, path); + LOG.debug("Converted node @{} with path {} was not put into map" + + " because a newer version is available", node.getRevisionId(), path); continue; } else { long revisionId = node.getRevisionId(); - LOG.debug("Converting node {} ({})", path, revisionId); + LOG.debug("Converting node {} (@{})", path, revisionId); - if (!commits.containsKey(revisionId) && nodeStore.getFromCache(revisionId) != null) { + if (!commits.containsKey(revisionId) && nodeStore.getFromCache(revisionId) == null) { LOG.debug("Fetching commit @{}", revisionId); FetchCommitAction action = new FetchCommitAction(nodeStore, revisionId); try { Index: src/main/java/org/apache/jackrabbit/mongomk/impl/command/CommitCommand.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/command/CommitCommand.java (revision 1430720) +++ src/main/java/org/apache/jackrabbit/mongomk/impl/command/CommitCommand.java (working copy) @@ -60,6 +60,8 @@ private MongoSync mongoSync; private Set nodes; private Long revisionId; + private final Long initialBaseRevisionId; + private Long baseRevisionId; private String branchId; /** @@ -71,14 +73,21 @@ public CommitCommand(MongoNodeStore nodeStore, Commit commit) { super(nodeStore); this.commit = (MongoCommit)commit; + this.initialBaseRevisionId = commit.getBaseRevisionId(); } @Override public Long execute() throws Exception { + int retries = 0; boolean success = false; do { mongoSync = new ReadAndIncHeadRevisionAction(nodeStore).execute(); revisionId = mongoSync.getNextRevisionId() - 1; + if (initialBaseRevisionId != null) { + baseRevisionId = initialBaseRevisionId; + } else { + baseRevisionId = mongoSync.getHeadRevisionId(); + } logger.debug("Committing @{} with diff: {}", revisionId, commit.getDiff()); readValidCommits(); readBranchIdFromBaseCommit(); @@ -90,9 +99,16 @@ new SaveNodesAction(nodeStore, nodes).execute(); new SaveCommitAction(nodeStore, commit).execute(); success = saveAndSetHeadRevision(); + if (!success) { + retries++; + } } while (!success); - logger.debug("Commit @{}: success", revisionId); + String msg = "Commit @{}: success"; + if (retries > 0) { + msg += " with {} retries."; + } + logger.debug(msg, revisionId, retries); return revisionId; } @@ -133,7 +149,7 @@ private void createMongoNodes() throws Exception { CommitCommandInstructionVisitor visitor = new CommitCommandInstructionVisitor( - nodeStore, mongoSync.getHeadRevisionId(), validCommits); + nodeStore, baseRevisionId, validCommits); visitor.setBranchId(branchId); for (Instruction instruction : commit.getInstructions()) { @@ -273,7 +289,7 @@ markAsFailed(); throw new ConflictingCommitException(message); } else { - logger.warn("Commit @{}: failed due to a conflicting commit." + logger.info("Commit @{}: failed due to a concurrent commit." + " Affected paths: {}", revisionId, commit.getAffectedPaths()); markAsFailed(); return false; @@ -303,6 +319,7 @@ DBObject query = QueryBuilder.start("_id").is(commit.getObjectId("_id")).get(); DBObject update = new BasicDBObject("$set", new BasicDBObject(MongoCommit.KEY_FAILED, Boolean.TRUE)); WriteResult writeResult = commitCollection.update(query, update); + nodeStore.evict(commit); if (writeResult.getError() != null) { // FIXME This is potentially a bug that we need to handle. throw new Exception(String.format("Update wasn't successful: %s", writeResult)); Index: src/main/java/org/apache/jackrabbit/mongomk/impl/command/CommitCommandNew.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/command/CommitCommandNew.java (revision 1430720) +++ src/main/java/org/apache/jackrabbit/mongomk/impl/command/CommitCommandNew.java (working copy) @@ -65,6 +65,8 @@ private MongoSync mongoSync; private Set nodes; private Long revisionId; + private final Long initialBaseRevisionId; + private Long baseRevisionId; private String branchId; /** @@ -76,14 +78,21 @@ public CommitCommandNew(MongoNodeStore nodeStore, Commit commit) { super(nodeStore); this.commit = (MongoCommit)commit; + this.initialBaseRevisionId = commit.getBaseRevisionId(); } @Override public Long execute() throws Exception { + int retries = 0; boolean success = false; do { mongoSync = new ReadAndIncHeadRevisionAction(nodeStore).execute(); revisionId = mongoSync.getNextRevisionId() - 1; + if (initialBaseRevisionId != null) { + baseRevisionId = initialBaseRevisionId; + } else { + baseRevisionId = mongoSync.getHeadRevisionId(); + } logger.debug("Committing @{} with diff: {}", revisionId, commit.getDiff()); readBranchIdFromBaseCommit(); createMongoNodes(); @@ -96,10 +105,16 @@ success = saveAndSetHeadRevision(); if (success) { cacheNodes(); + } else { + retries++; } } while (!success); - logger.debug("Commit @{}: success", revisionId); + String msg = "Commit @{}: success"; + if (retries > 0) { + msg += " with {} retries."; + } + logger.debug(msg, revisionId, retries); return revisionId; } @@ -134,7 +149,7 @@ private void createMongoNodes() throws Exception { CommitCommandInstructionVisitor visitor = new CommitCommandInstructionVisitor( - nodeStore, mongoSync.getHeadRevisionId(), null); + nodeStore, baseRevisionId, null); visitor.setBranchId(branchId); for (Instruction instruction : commit.getInstructions()) { @@ -294,7 +309,7 @@ markAsFailed(); throw new ConflictingCommitException(message); } else { - logger.warn("Commit @{}: failed due to a conflicting commit." + logger.info("Commit @{}: failed due to a concurrent commit." + " Affected paths: {}", revisionId, commit.getAffectedPaths()); markAsFailed(); return false; @@ -324,6 +339,7 @@ DBObject query = QueryBuilder.start("_id").is(commit.getObjectId("_id")).get(); DBObject update = new BasicDBObject("$set", new BasicDBObject(MongoCommit.KEY_FAILED, Boolean.TRUE)); WriteResult writeResult = commitCollection.update(query, update); + nodeStore.evict(commit); if (writeResult.getError() != null) { // FIXME This is potentially a bug that we need to handle. throw new Exception(String.format("Update wasn't successful: %s", writeResult)); Index: src/main/java/org/apache/jackrabbit/mongomk/impl/instruction/CommitCommandInstructionVisitor.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/instruction/CommitCommandInstructionVisitor.java (revision 1430720) +++ src/main/java/org/apache/jackrabbit/mongomk/impl/instruction/CommitCommandInstructionVisitor.java (working copy) @@ -41,7 +41,8 @@ */ public class CommitCommandInstructionVisitor implements InstructionVisitor { - private final long headRevisionId; + // the revision this commit is based on + private final long baseRevisionId; private final MongoNodeStore nodeStore; private final Map pathNodeMap; private final List validCommits; @@ -52,13 +53,14 @@ * Creates {@code CommitCommandInstructionVisitor} * * @param nodeStore Node store. - * @param headRevisionId Head revision. + * @param baseRevisionId the revision this commit is based on * @param validCommits */ - public CommitCommandInstructionVisitor(MongoNodeStore nodeStore, long headRevisionId, - List validCommits) { + public CommitCommandInstructionVisitor(MongoNodeStore nodeStore, + long baseRevisionId, + List validCommits) { this.nodeStore = nodeStore; - this.headRevisionId = headRevisionId; + this.baseRevisionId = baseRevisionId; this.validCommits = validCommits; pathNodeMap = new HashMap(); } @@ -93,9 +95,7 @@ } String parentNodePath = PathUtils.getParentPath(nodePath); - // FIXME - Performance - //MongoNode parent = getStoredNode(parentNodePath); - MongoNode parent = getStagedNode(parentNodePath); + MongoNode parent = getStoredNode(parentNodePath); if (parent.childExists(nodeName)) { throw new RuntimeException("There's already a child node with name '" + nodeName + "'"); } @@ -107,9 +107,7 @@ public void visit(SetPropertyInstruction instruction) { String key = instruction.getKey(); Object value = instruction.getValue(); - // FIXME - Performance - //MongoNode node = getStoredNode(instruction.getPath()); - MongoNode node = getStagedNode(instruction.getPath()); + MongoNode node = getStoredNode(instruction.getPath()); if (value == null) { node.removeProp(key); } else { @@ -123,14 +121,12 @@ checkAbsolutePath(nodePath); String parentPath = PathUtils.getParentPath(nodePath); - MongoNode parent = getStagedNode(parentPath); String nodeName = PathUtils.getName(nodePath); - // See OAK-507 -// MongoNode parent = getStoredNode(parentPath); -// if (!parent.childExists(nodeName)) { -// throw new RuntimeException("Node " + nodeName -// + " does not exists at parent path: " + parentPath); -// } + MongoNode parent = getStoredNode(parentPath); + if (!parent.childExists(nodeName)) { + throw new RuntimeException("Node " + nodeName + + " does not exists at parent path: " + parentPath); + } parent.removeChild(nodeName); } @@ -162,7 +158,7 @@ // First, copy the existing nodes. Map nodesToCopy = new FetchNodesActionNew(nodeStore, - srcPath, FetchNodesActionNew.LIMITLESS_DEPTH, headRevisionId).execute(); + srcPath, FetchNodesActionNew.LIMITLESS_DEPTH, baseRevisionId).execute(); for (MongoNode nodeMongo : nodesToCopy.values()) { String oldPath = nodeMongo.getPath(); String oldPathRel = PathUtils.relativize(srcPath, oldPath); @@ -209,7 +205,7 @@ // First, copy the existing nodes. Map nodesToCopy = new FetchNodesActionNew(nodeStore, - srcPath, FetchNodesActionNew.LIMITLESS_DEPTH, headRevisionId).execute(); + srcPath, FetchNodesActionNew.LIMITLESS_DEPTH, baseRevisionId).execute(); for (MongoNode nodeMongo : nodesToCopy.values()) { String oldPath = nodeMongo.getPath(); String oldPathRel = PathUtils.relativize(srcPath, oldPath); @@ -258,7 +254,7 @@ // First need to check that the path is indeed valid. NodeExistsCommand existCommand = new NodeExistsCommand(nodeStore, - path, headRevisionId); + path, baseRevisionId); existCommand.setBranchId(branchId); boolean exists = false; try { @@ -266,7 +262,7 @@ } catch (Exception ignore) {} if (!exists) { - throw new NotFoundException(path + " @rev" + headRevisionId); + throw new NotFoundException(path + " @rev" + baseRevisionId); } node = existCommand.getNode(); node.removeField("_id"); Index: src/main/java/org/apache/jackrabbit/mongomk/impl/model/MongoNode.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/model/MongoNode.java (revision 1430720) +++ src/main/java/org/apache/jackrabbit/mongomk/impl/model/MongoNode.java (working copy) @@ -131,6 +131,13 @@ put(KEY_REVISION_ID, revisionId); } + @Override + public MongoNode copy() { + MongoNode copy = new MongoNode(); + copy.putAll((Map) super.copy()); + return copy; + } + //-------------------------------------------------------------------------- // // These properties are used to keep track of changes but not persisted Index: src/main/java/org/apache/jackrabbit/mongomk/impl/MongoNodeStore.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/MongoNodeStore.java (revision 1430720) +++ src/main/java/org/apache/jackrabbit/mongomk/impl/MongoNodeStore.java (working copy) @@ -200,6 +200,17 @@ } /** + * Evicts the commit from the {@link #commitCache}. + * + * @param commit the commit. + */ + public void evict(MongoCommit commit) { + if (commitCache.remove(commit.getRevisionId()) != null) { + LOG.debug("Removed commit {} from cache", commit.getRevisionId()); + } + } + + /** * Returns the commit from the cache or null if the commit is not in the cache. * * @param revisionId Commit revision id. @@ -225,7 +236,7 @@ String key = path + "*" + branchId + "*" + revisionId; if (!nodeCache.containsKey(key)) { LOG.debug("Adding node to cache: {}", key); - nodeCache.put(key, node); + nodeCache.put(key, node.copy()); } } Index: src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKBranchMergeTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKBranchMergeTest.java (revision 1430720) +++ src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKBranchMergeTest.java (working copy) @@ -330,19 +330,17 @@ } private String addNodes(String rev, String...nodes) { - String newRev = rev; for (String node : nodes) { - newRev = mk.commit("", "+\"" + node + "\":{}", rev, ""); + rev = mk.commit("", "+\"" + node + "\":{}", rev, ""); } - return newRev; + return rev; } private String removeNodes(String rev, String...nodes) { - String newRev = rev; for (String node : nodes) { - newRev = mk.commit("", "-\"" + node + "\"", rev, ""); + rev = mk.commit("", "-\"" + node + "\"", rev, ""); } - return newRev; + return rev; } private String setProp(String rev, String prop, Object value) { Index: src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitRemoveTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitRemoveTest.java (revision 1430720) +++ src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitRemoveTest.java (working copy) @@ -6,7 +6,6 @@ import static org.junit.Assert.fail; import org.apache.jackrabbit.mongomk.BaseMongoMicroKernelTest; -import org.junit.Ignore; import org.junit.Test; /** @@ -28,8 +27,6 @@ } @Test - @Ignore - // According to OAK-507, this should not fail. public void removeNonExistentNode() throws Exception { try { mk.commit("/", "-\"a\"", null, null); @@ -48,9 +45,10 @@ @Test public void removeAndAddNode() throws Exception { String base = mk.commit("", "+\"/a\":{}", null, null); - mk.commit("", "-\"/a\"", base, null); + String rev = mk.commit("", "-\"/a\"", base, null); assertTrue(mk.nodeExists("/a", base)); - mk.commit("", "+\"/a\":{}", base, null); + assertFalse(mk.nodeExists("/a", rev)); + mk.commit("", "+\"/a\":{}", rev, null); } @Test