jackrabbit-oak-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mdue...@apache.org
Subject svn commit: r1506027 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak: kernel/KernelNodeStore.java kernel/KernelNodeStoreBranch.java plugins/segment/SegmentNodeStoreBranch.java
Date Tue, 23 Jul 2013 13:08:51 GMT
Author: mduerig
Date: Tue Jul 23 13:08:51 2013
New Revision: 1506027

URL: http://svn.apache.org/r1506027
Log:
OAK-927 Concurrent commits may cause duplicate observation events
- call commit hook with correct base revision
- use explicit lock object from node store to synchronise concurrent merge calls
credits to Chetan for spotting these issues

Modified:
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStore.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeStoreBranch.java

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStore.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStore.java?rev=1506027&r1=1506026&r2=1506027&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStore.java
(original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStore.java
Tue Jul 23 13:08:51 2013
@@ -16,9 +16,14 @@
  */
 package org.apache.jackrabbit.oak.kernel;
 
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
@@ -29,18 +34,14 @@ import com.google.common.cache.LoadingCa
 import com.google.common.cache.Weigher;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.SettableFuture;
-
 import org.apache.jackrabbit.mk.api.MicroKernel;
 import org.apache.jackrabbit.mk.api.MicroKernelException;
+import org.apache.jackrabbit.oak.cache.CacheStats;
 import org.apache.jackrabbit.oak.spi.commit.EmptyObserver;
 import org.apache.jackrabbit.oak.spi.commit.Observer;
 import org.apache.jackrabbit.oak.spi.state.AbstractNodeStore;
 import org.apache.jackrabbit.oak.spi.state.NodeState;
 import org.apache.jackrabbit.oak.spi.state.NodeStoreBranch;
-import org.apache.jackrabbit.oak.cache.CacheStats;
-
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkNotNull;
 
 /**
  * {@code NodeStore} implementations against {@link MicroKernel}.
@@ -65,6 +66,11 @@ public class KernelNodeStore extends Abs
     private final CacheStats cacheStats;
 
     /**
+     * Lock passed to branches for coordinating merges
+     */
+    private final Lock mergeLock = new ReentrantLock();
+
+    /**
      * State of the current root node.
      */
     private KernelNodeState root;
@@ -140,7 +146,7 @@ public class KernelNodeStore extends Abs
 
     @Override
     public NodeStoreBranch branch() {
-        return new KernelNodeStoreBranch(this, getRoot());
+        return new KernelNodeStoreBranch(this, getRoot(), mergeLock);
     }
 
     /**

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java?rev=1506027&r1=1506026&r2=1506027&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java
(original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java
Tue Jul 23 13:08:51 2013
@@ -23,6 +23,8 @@ import static org.apache.jackrabbit.oak.
 import static org.apache.jackrabbit.oak.commons.PathUtils.getName;
 import static org.apache.jackrabbit.oak.commons.PathUtils.getParentPath;
 
+import java.util.concurrent.locks.Lock;
+
 import org.apache.jackrabbit.mk.api.MicroKernel;
 import org.apache.jackrabbit.mk.api.MicroKernelException;
 import org.apache.jackrabbit.oak.api.CommitFailedException;
@@ -43,6 +45,11 @@ class KernelNodeStoreBranch extends Abst
     /** The underlying store to which this branch belongs */
     private final KernelNodeStore store;
 
+    /**
+     * Lock for coordinating concurrent merge operations
+     */
+    private final Lock mergeLock;
+
     /** Root state of the base revision of this branch */
     private KernelNodeState base;
 
@@ -55,10 +62,11 @@ class KernelNodeStoreBranch extends Abst
     /** Number of updates to this branch via {@link #setRoot(NodeState)} */
     private int updates = 0;
 
-    KernelNodeStoreBranch(KernelNodeStore store, KernelNodeState root) {
+    KernelNodeStoreBranch(KernelNodeStore store, KernelNodeState root, Lock mergeLock) {
         this.store = store;
         this.base = root;
         this.head = root;
+        this.mergeLock = mergeLock;
     }
 
     @Override
@@ -138,46 +146,46 @@ class KernelNodeStoreBranch extends Abst
     @Override
     public NodeState merge(CommitHook hook, PostCommitHook committed) throws CommitFailedException
{
         checkNotMerged();
-        synchronized (this) {
+        NodeState oldRoot = head;
+        mergeLock.lock();
+        try {
             rebase();
             NodeState toCommit = checkNotNull(hook).processCommit(base, head);
-            NodeState oldRoot = head;
             head = toCommit;
-
-            try {
-                if (head.equals(base)) {
-                    // Nothing was written to this branch: return base state
-                    head = null;  // Mark as merged
-                    committed.contentChanged(base, base);
-                    return base;
+            if (head.equals(base)) {
+                // Nothing was written to this branch: return base state
+                head = null;  // Mark as merged
+                committed.contentChanged(base, base);
+                return base;
+            } else {
+                NodeState newRoot;
+                JsopDiff diff = new JsopDiff(store);
+                if (headRevision == null) {
+                    // no branch created yet, commit directly
+                    head.compareAgainstBaseState(base, diff);
+                    newRoot = store.commit(diff.toString(), base.getRevision());
                 } else {
-                    NodeState newRoot;
-                    JsopDiff diff = new JsopDiff(store);
-                    if (headRevision == null) {
-                        // no branch created yet, commit directly
-                        head.compareAgainstBaseState(base, diff);
-                        newRoot = store.commit(diff.toString(), base.getRevision());
-                    } else {
-                        // commit into branch and merge
-                        head.compareAgainstBaseState(store.getRootState(headRevision), diff);
-                        String jsop = diff.toString();
-                        if (!jsop.isEmpty()) {
-                            headRevision = store.getKernel().commit(
-                                    "", jsop, headRevision, null);
-                        }
-                        newRoot = store.merge(headRevision);
-                        headRevision = null;
+                    // commit into branch and merge
+                    head.compareAgainstBaseState(store.getRootState(headRevision), diff);
+                    String jsop = diff.toString();
+                    if (!jsop.isEmpty()) {
+                        headRevision = store.getKernel().commit(
+                                "", jsop, headRevision, null);
                     }
-                    head = null;  // Mark as merged
-                    committed.contentChanged(base, newRoot);
-                    return newRoot;
+                    newRoot = store.merge(headRevision);
+                    headRevision = null;
                 }
-            } catch (MicroKernelException e) {
-                head = oldRoot;
-                throw new CommitFailedException(
-                        "Kernel", 1,
-                        "Failed to merge changes to the underlying MicroKernel", e);
+                head = null;  // Mark as merged
+                committed.contentChanged(base, newRoot);
+                return newRoot;
             }
+        } catch (MicroKernelException e) {
+            head = oldRoot;
+            throw new CommitFailedException(
+                    "Kernel", 1,
+                    "Failed to merge changes to the underlying MicroKernel", e);
+        } finally {
+            mergeLock.unlock();
         }
     }
 

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeStoreBranch.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeStoreBranch.java?rev=1506027&r1=1506026&r2=1506027&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeStoreBranch.java
(original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeStoreBranch.java
Tue Jul 23 13:08:51 2013
@@ -118,9 +118,10 @@ class SegmentNodeStoreBranch extends Abs
                 // someone else has a pessimistic lock on the journal,
                 // so we should not try to commit anything
             } else if (store.setHead(base, newHead)) {
+                NodeState previousBase = base;
                 base = newHead;
                 head = newHead;
-                committed.contentChanged(originalBase.getChildNode(ROOT), newHead.getChildNode(ROOT));
+                committed.contentChanged(previousBase.getChildNode(ROOT), newHead.getChildNode(ROOT));
                 return -1;
             }
 
@@ -176,9 +177,10 @@ class SegmentNodeStoreBranch extends Abs
                             writer.writeNode(builder.getNodeState());
                     writer.flush();
                     if (store.setHead(after, newHead)) {
+                        NodeState previousBase = base;
                         base = newHead;
                         head = newHead;
-                        committed.contentChanged(originalBase.getChildNode(ROOT), newHead.getChildNode(ROOT));
+                        committed.contentChanged(previousBase.getChildNode(ROOT), newHead.getChildNode(ROOT));
                         return;
                     } else {
                         // something else happened, perhaps a timeout, so



Mime
View raw message