jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Dürig <mdue...@apache.org>
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 18:58:47 GMT


On 16.10.13 6:36 , Jukka Zitting wrote:
> 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

Well there is the extra benefit for transporting that info across 
cluster nodes. See below.

and
> potentially troublesome due to the extra contention and possible
> conflicts on the commitinfo node.

Conflicts at that point cannot occur within a single cluster node since 
we are already serialised here. Conflicts might however occur between 
different cluster nodes. Again, see below.

>
>>> 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.

Yes this should work (even though I'm not too fond of using thread locals).

>
>> 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.

That's why I said limited ;-) However, in the case of a conflict on the 
commit info node it should be rather easy to come up with a merge 
strategy: add a merge commit for the cluster sync having a new commit 
info node which consists of the union of the values of the individual 
commit infos.

Michael


>
> 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