Index: src/main/java/org/apache/jackrabbit/mongomk/impl/action/FetchCommitsAction.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/action/FetchCommitsAction.java (revision 1431321) +++ 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 1431321) +++ 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 1431321) +++ 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 1431321) +++ src/main/java/org/apache/jackrabbit/mongomk/impl/command/CommitCommandNew.java (working copy) @@ -29,7 +29,6 @@ import org.apache.jackrabbit.mongomk.api.model.Commit; import org.apache.jackrabbit.mongomk.impl.MongoNodeStore; import org.apache.jackrabbit.mongomk.impl.action.FetchCommitAction; -import org.apache.jackrabbit.mongomk.impl.action.FetchNodesActionNew; import org.apache.jackrabbit.mongomk.impl.action.ReadAndIncHeadRevisionAction; import org.apache.jackrabbit.mongomk.impl.action.SaveAndSetHeadRevisionAction; import org.apache.jackrabbit.mongomk.impl.action.SaveCommitAction; @@ -63,8 +62,10 @@ private Set affectedPaths; private Map existingNodes; private MongoSync mongoSync; - private Set nodes; + private Map nodes; private Long revisionId; + private final Long initialBaseRevisionId; + private Long baseRevisionId; private String branchId; /** @@ -76,14 +77,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(); @@ -91,15 +99,21 @@ readExistingNodes(); mergeNodes(); prepareMongoNodes(); - new SaveNodesAction(nodeStore, nodes).execute(); + new SaveNodesAction(nodeStore, nodes.values()).execute(); new SaveCommitAction(nodeStore, commit).execute(); 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,16 +148,15 @@ private void createMongoNodes() throws Exception { CommitCommandInstructionVisitor visitor = new CommitCommandInstructionVisitor( - nodeStore, mongoSync.getHeadRevisionId(), null); + nodeStore, baseRevisionId, null); visitor.setBranchId(branchId); for (Instruction instruction : commit.getInstructions()) { instruction.accept(visitor); } - Map pathNodeMap = visitor.getPathNodeMap(); - affectedPaths = pathNodeMap.keySet(); - nodes = new HashSet(pathNodeMap.values()); + nodes = visitor.getPathNodeMap(); + affectedPaths = nodes.keySet(); } private void prepareCommit() throws Exception { @@ -164,58 +177,56 @@ // } // FIXME - Performance, This seems to be faster for commits than the old method. - private void readExistingNodes() { + private void readExistingNodes() throws Exception { if (affectedPaths == null || affectedPaths.isEmpty()) { existingNodes = Collections.emptyMap(); } existingNodes = new HashMap(); for (String path : affectedPaths) { - FetchNodesActionNew action = new FetchNodesActionNew(nodeStore, path, - 0, mongoSync.getHeadRevisionId()); - action.setBranchId(branchId); - Map result = action.execute(); - MongoNode node = result.get(path); - if (node != null) { - existingNodes.put(path, node); + NodeExistsCommand command = new NodeExistsCommand( + nodeStore, path, mongoSync.getHeadRevisionId()); + command.setBranchId(branchId); + if (command.execute()) { + existingNodes.put(path, command.getNode()); } } } private void mergeNodes() { - for (MongoNode existingNode : existingNodes.values()) { - for (MongoNode committingNode : nodes) { - if (existingNode.getPath().equals(committingNode.getPath())) { - if(logger.isDebugEnabled()){ - logger.debug("Found existing node to merge: {}", existingNode.getPath()); - logger.debug("Existing node: {}", existingNode); - logger.debug("Committing node: {}", committingNode); - } - Map existingProperties = existingNode.getProperties(); - if (!existingProperties.isEmpty()) { - committingNode.setProperties(existingProperties); + for (MongoNode committingNode : nodes.values()) { + MongoNode existingNode = existingNodes.get(committingNode.getPath()); + if (existingNode != null) { + if(logger.isDebugEnabled()){ + logger.debug("Found existing node to merge: {}", existingNode.getPath()); + logger.debug("Existing node: {}", existingNode); + logger.debug("Committing node: {}", committingNode); + } + Map existingProperties = existingNode.getProperties(); + if (!existingProperties.isEmpty()) { + committingNode.setProperties(existingProperties); - logger.debug("Merged properties for {}: {}", existingNode.getPath(), - existingProperties); - } + logger.debug("Merged properties for {}: {}", existingNode.getPath(), + existingProperties); + } - List existingChildren = existingNode.getChildren(); - if (existingChildren != null) { - committingNode.setChildren(existingChildren); + List existingChildren = existingNode.getChildren(); + if (existingChildren != null) { + committingNode.setChildren(existingChildren); - logger.debug("Merged children for {}: {}", existingNode.getPath(), existingChildren); - } + logger.debug("Merged children for {}: {}", existingNode.getPath(), existingChildren); + } - logger.debug("Merged node for {}: {}", existingNode.getPath(), committingNode); - - break; - } + logger.debug("Merged node for {}: {}", existingNode.getPath(), committingNode); + } else { + // FIXME: this may also mean a node we modify has + // been removed in the meantime } } } private void prepareMongoNodes() { - for (MongoNode committingNode : nodes) { + for (MongoNode committingNode : nodes.values()) { logger.debug("Preparing children (added and removed) of {}", committingNode.getPath()); logger.debug("Committing node: {}", committingNode); @@ -294,8 +305,7 @@ markAsFailed(); throw new ConflictingCommitException(message); } else { - logger.warn("Commit @{}: failed due to a conflicting commit." - + " Affected paths: {}", revisionId, commit.getAffectedPaths()); + logger.info("Commit @{}: failed due to a concurrent commit." + " Affected paths: {}", revisionId, commit.getAffectedPaths()); markAsFailed(); return false; } @@ -324,6 +334,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)); @@ -331,7 +342,7 @@ } private void cacheNodes() { - for (MongoNode node : nodes) { + for (MongoNode node : nodes.values()) { nodeStore.cache(node); } } Index: src/main/java/org/apache/jackrabbit/mongomk/impl/instruction/CommitCommandInstructionVisitor.java =================================================================== --- src/main/java/org/apache/jackrabbit/mongomk/impl/instruction/CommitCommandInstructionVisitor.java (revision 1431321) +++ 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); } @@ -161,8 +157,10 @@ } // First, copy the existing nodes. - Map nodesToCopy = new FetchNodesActionNew(nodeStore, - srcPath, FetchNodesActionNew.LIMITLESS_DEPTH, headRevisionId).execute(); + FetchNodesActionNew action = new FetchNodesActionNew(nodeStore, + srcPath, FetchNodesActionNew.LIMITLESS_DEPTH, baseRevisionId); + action.setBranchId(branchId); + Map nodesToCopy = action.execute(); for (MongoNode nodeMongo : nodesToCopy.values()) { String oldPath = nodeMongo.getPath(); String oldPathRel = PathUtils.relativize(srcPath, oldPath); @@ -208,8 +206,10 @@ } // First, copy the existing nodes. - Map nodesToCopy = new FetchNodesActionNew(nodeStore, - srcPath, FetchNodesActionNew.LIMITLESS_DEPTH, headRevisionId).execute(); + FetchNodesActionNew action = new FetchNodesActionNew(nodeStore, + srcPath, FetchNodesActionNew.LIMITLESS_DEPTH, baseRevisionId); + action.setBranchId(branchId); + Map nodesToCopy = action.execute(); for (MongoNode nodeMongo : nodesToCopy.values()) { String oldPath = nodeMongo.getPath(); String oldPathRel = PathUtils.relativize(srcPath, oldPath); @@ -258,7 +258,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 +266,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 1431321) +++ 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 1431321) +++ 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 1431321) +++ 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/MongoMKBranchTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKBranchTest.java (revision 1431321) +++ src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKBranchTest.java (working copy) @@ -19,10 +19,10 @@ import org.apache.jackrabbit.mongomk.BaseMongoMicroKernelTest; import org.json.simple.JSONObject; import org.json.simple.parser.JSONParser; -import org.junit.Ignore; import org.junit.Test; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; /** * MongoMKBranchTest performs a test to check if commits @@ -60,4 +60,19 @@ JSONObject obj = (JSONObject) parser.parse(json); assertFalse(obj.containsKey("foo")); } + + @Test + public void movesInBranch() throws Exception { + String branchRev = mk.branch(null); + branchRev = mk.commit("/", "+\"a\":{}", branchRev, null); + branchRev = mk.commit("/a", "^\"foo\":1", branchRev, null); + branchRev = mk.commit("/", ">\"a\" : \"b\"", branchRev, null); + branchRev = mk.commit("/", ">\"b\" : \"a\"", branchRev, null); + mk.merge(branchRev, null); + + String json = mk.getNodes("/a", null, 0, 0, -1, null); + JSONParser parser = new JSONParser(); + JSONObject obj = (JSONObject) parser.parse(json); + assertTrue(obj.containsKey("foo")); + } } Index: src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitAddTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitAddTest.java (revision 1431321) +++ src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitAddTest.java (working copy) @@ -75,7 +75,6 @@ } @Test - @Ignore // FIXME - due to CommitCommandInstructionVisitor add node change. public void addDuplicateNode() throws Exception { mk.commit("/", "+\"a\" : {}", null, null); try { @@ -138,7 +137,6 @@ } @Test - @Ignore // FIXME - due to CommitCommandInstructionVisitor add node change. public void setPropertyWithoutAddingNode() throws Exception { try { mk.commit("/", "^\"a/key1\" : \"value1\"", null, null); Index: src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitRemoveTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitRemoveTest.java (revision 1431321) +++ src/test/java/org/apache/jackrabbit/mongomk/impl/MongoMKCommitRemoveTest.java (working copy) @@ -6,7 +6,7 @@ import static org.junit.Assert.fail; import org.apache.jackrabbit.mongomk.BaseMongoMicroKernelTest; -import org.junit.Ignore; +import org.json.simple.JSONObject; import org.junit.Test; /** @@ -28,8 +28,6 @@ } @Test - @Ignore - // According to OAK-507, this should not fail. public void removeNonExistentNode() throws Exception { try { mk.commit("/", "-\"a\"", null, null); @@ -48,9 +46,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