jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jukka Zitting <jukka.zitt...@gmail.com>
Subject Re: svn commit: r1532782 [1/2] - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/ oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/ oak-core/src/main/jav...
Date Wed, 16 Oct 2013 16:36:35 GMT
Hi,

On Wed, Oct 16, 2013 at 11:47 AM, Michael Dürig <mduerig@apache.org> wrote:
> On 16.10.13 5:23 , Jukka Zitting wrote:
>> I don't think this is a good idea.
>
> Why?

You're essentially passing information from Root.commit() a few levels
down the stack to ChangeDispatcher.localCommit(). Routing that
information through the persisted state seems quite unnecessary and
potentially troublesome due to the extra contention and possible
conflicts on the commitinfo node.

>> Instead I'd use a ThreadLocal or
>> just pass the extra information as an extra argument in the commit
>> call.
>
> That's what I wanted to do initially, but I failed to come up with a clean
> way to pass the session specific information down to the NodeStore
> implementations. I'd be open for alternative solutions ;-)

See the patch below for a quick and dirty approach.

> Besides the current approach allows for limited support of said session
> specific information to be transported across cluster nodes.

You can't do that reliably. Say you have concurrent commits going on
in separate cluster nodes and you only see the result of merging those
changes. At that point it's impossible to tell which changes are
related to the commitinfo you're seeing.

BR,

Jukka Zitting


diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentSessionImpl.java
b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentSessionImpl.java
index 96db894..3e179c0 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentSessionImpl.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ContentSessionImpl.java
@@ -27,14 +27,13 @@ import javax.annotation.Nonnull;
 import javax.security.auth.login.LoginException;

 import org.apache.jackrabbit.oak.api.AuthInfo;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
 import org.apache.jackrabbit.oak.api.ContentSession;
 import org.apache.jackrabbit.oak.api.Root;
 import org.apache.jackrabbit.oak.plugins.observation.ChangeDispatcher.Listener;
-import org.apache.jackrabbit.oak.plugins.observation.CommitInfoEditorProvider;
+import org.apache.jackrabbit.oak.plugins.observation.ChangeDispatcher;
 import org.apache.jackrabbit.oak.plugins.observation.Observable;
 import org.apache.jackrabbit.oak.spi.commit.CommitHook;
-import org.apache.jackrabbit.oak.spi.commit.CompositeHook;
-import org.apache.jackrabbit.oak.spi.commit.EditorHook;
 import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
 import org.apache.jackrabbit.oak.spi.security.SecurityProvider;
 import org.apache.jackrabbit.oak.spi.security.authentication.LoginContext;
@@ -107,10 +106,7 @@ class ContentSessionImpl implements
ContentSession, Observable {
     public Root getLatestRoot() {
         checkLive();

-        EditorHook commitInfoEditor = new EditorHook(
-                new CommitInfoEditorProvider(sessionName,
getAuthInfo().getUserID()));
-
-        return new AbstractRoot(store, new CompositeHook(hook,
commitInfoEditor),
+        return new AbstractRoot(store, hook,
                 workspaceName, loginContext.getSubject(),
securityProvider, indexProvider) {
             @Override
             protected void checkLive() {
@@ -121,6 +117,17 @@ class ContentSessionImpl implements
ContentSession, Observable {
             public ContentSession getContentSession() {
              return ContentSessionImpl.this;
             }
+
+            @Override
+            public void commit(final CommitHook... hooks)
+                    throws CommitFailedException {
+                ChangeDispatcher.currentSession.set(ContentSessionImpl.this);
+                try {
+                    super.commit(hooks);
+                } finally {
+                    ChangeDispatcher.currentSession.remove();
+                }
+            }
         };
     }

diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/ChangeDispatcher.java
b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/ChangeDispatcher.java
index c94209b..f889f86 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/ChangeDispatcher.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/ChangeDispatcher.java
@@ -21,8 +21,6 @@ package org.apache.jackrabbit.oak.plugins.observation;
 import static com.google.common.base.Objects.toStringHelper;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
-import static org.apache.jackrabbit.oak.api.Type.LONG;
-import static org.apache.jackrabbit.oak.api.Type.STRING;

 import java.util.Queue;
 import java.util.Set;
@@ -34,7 +32,8 @@ import javax.annotation.Nonnull;
 import com.google.common.base.Objects;
 import com.google.common.collect.Queues;
 import com.google.common.collect.Sets;
-import org.apache.jackrabbit.oak.api.PropertyState;
+
+import org.apache.jackrabbit.oak.api.ContentSession;
 import org.apache.jackrabbit.oak.spi.state.NodeState;
 import org.apache.jackrabbit.oak.spi.state.NodeStore;

@@ -61,6 +60,10 @@ import org.apache.jackrabbit.oak.spi.state.NodeStore;
  * into a change dispatcher.
  */
 public class ChangeDispatcher {
+
+    public static final ThreadLocal<ContentSession> currentSession =
+            new ThreadLocal<ContentSession>();
+
     private final Set<Listener> listeners = Sets.newHashSet();
     private final NodeStore store;

@@ -158,13 +161,13 @@ public class ChangeDispatcher {

     private synchronized void externalChange(NodeState root) {
         if (!root.equals(previousRoot)) {
-            add(new ChangeSet(previousRoot, root, true));
+            add(new ChangeSet(previousRoot, root, null));
             previousRoot = root;
         }
     }

     private synchronized void internalChange(NodeState root) {
-        add(new ChangeSet(previousRoot, root, false));
+        add(new ChangeSet(previousRoot, root, currentSession.get()));
         previousRoot = root;
     }

@@ -238,11 +241,22 @@ public class ChangeDispatcher {
         private final NodeState before;
         private final NodeState after;
         private final boolean isExternal;
+        private final String sessionId;
+        private final String userId;
+        private final long date = System.currentTimeMillis();

-        ChangeSet(NodeState before, NodeState after, boolean isExternal) {
+        ChangeSet(NodeState before, NodeState after, ContentSession session) {
             this.before = before;
             this.after = after;
-            this.isExternal = isExternal;
+            if (session != null) {
+                this.isExternal = false;
+                this.sessionId = session.toString();
+                this.userId = session.getAuthInfo().getUserID();
+            } else {
+                this.isExternal = true;
+                this.sessionId = null;
+                this.userId = null;
+            }
         }

         public boolean isExternal() {
@@ -255,17 +269,16 @@ public class ChangeDispatcher {

         @CheckForNull
         public String getSessionId() {
-            return getStringOrNull(getCommitInfo(after),
CommitInfoEditorProvider.SESSION_ID);
+            return sessionId;
         }

         @CheckForNull
         public String getUserId() {
-            return getStringOrNull(getCommitInfo(after),
CommitInfoEditorProvider.USER_ID);
+            return userId;
         }

         public long getDate() {
-            PropertyState property =
getCommitInfo(after).getProperty(CommitInfoEditorProvider.TIME_STAMP);
-            return property == null ? 0 : property.getValue(LONG);
+            return date;
         }

         /**
@@ -291,9 +304,9 @@ public class ChangeDispatcher {
             return toStringHelper(this)
                 .add("base", before)
                 .add("head", after)
-                .add(CommitInfoEditorProvider.USER_ID, getUserId())
-                .add(CommitInfoEditorProvider.TIME_STAMP, getDate())
-                .add(CommitInfoEditorProvider.SESSION_ID, getSessionId())
+                .add("date", date)
+                .add("user", userId)
+                .add("session", sessionId)
                 .add("external", isExternal)
                 .toString();
         }
@@ -317,14 +330,5 @@ public class ChangeDispatcher {
             return 31 * before.hashCode() + after.hashCode();
         }

-        private static String getStringOrNull(NodeState commitInfo,
String name) {
-            PropertyState property = commitInfo.getProperty(name);
-            return property == null ? null : property.getValue(STRING);
-        }
-
-        private static NodeState getCommitInfo(NodeState after) {
-            return after.getChildNode(CommitInfoEditorProvider.COMMIT_INFO);
-        }
-
     }
 }

Mime
View raw message