You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by ju...@apache.org on 2014/01/15 03:48:59 UTC
svn commit: r1558270 - in
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
core/AbstractRoot.java plugins/memory/MutableNodeState.java
spi/state/EqualsDiff.java
Author: jukka
Date: Wed Jan 15 02:48:59 2014
New Revision: 1558270
URL: http://svn.apache.org/r1558270
Log:
OAK-659: Move purge logic for transient changes below the NodeBuilder interface
Remove the extra equals() check from AbstractRoot.rebase().
Fix the MutableNodeState.isModified() check to correctly handle the case where the modifications have been flushed.
Added:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNodeState.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java?rev=1558270&r1=1558269&r2=1558270&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java Wed Jan 15 02:48:59 2014
@@ -237,12 +237,10 @@ public abstract class AbstractRoot imple
@Override
public void rebase() {
checkLive();
- if (!store.getRoot().equals(getBaseState())) { // TODO: do we need this?
- store.rebase(builder);
- secureBuilder.baseChanged();
- if (permissionProvider.hasValue()) {
- permissionProvider.get().refresh();
- }
+ store.rebase(builder);
+ secureBuilder.baseChanged();
+ if (permissionProvider.hasValue()) {
+ permissionProvider.get().refresh();
}
}
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNodeState.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNodeState.java?rev=1558270&r1=1558269&r2=1558270&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNodeState.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNodeState.java Wed Jan 15 02:48:59 2014
@@ -31,6 +31,7 @@ import javax.annotation.Nonnull;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.spi.state.AbstractNodeState;
import org.apache.jackrabbit.oak.spi.state.ChildNodeEntry;
+import org.apache.jackrabbit.oak.spi.state.EqualsDiff;
import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
import org.apache.jackrabbit.oak.spi.state.NodeState;
import org.apache.jackrabbit.oak.spi.state.NodeStateDiff;
@@ -120,7 +121,7 @@ class MutableNodeState extends AbstractN
if (!exists()) {
return false;
} else if (nodes.isEmpty() && properties.isEmpty()) {
- return false;
+ return EqualsDiff.modified(before, base);
}
// was a child node added or removed?
Added: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java?rev=1558270&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java (added)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java Wed Jan 15 02:48:59 2014
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.spi.state;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+
+/**
+ * Helper class for comparing the equality of node states based on the
+ * content diff mechanism.
+ */
+public class EqualsDiff implements NodeStateDiff {
+
+ /**
+ * Diffs the given node states and returns {@code true} if there are no
+ * differences.
+ *
+ * @param before before state
+ * @param after after state
+ * @return {@code true} if the states are equal, {@code false} otherwise
+ */
+ public static boolean equals(NodeState before, NodeState after) {
+ return before.exists() == after.exists()
+ && after.compareAgainstBaseState(before, new EqualsDiff());
+ }
+
+ /**
+ * Diffs the given node states and returns {@code true} if there are
+ * differences within the properties or direct child nodes.
+ *
+ * @param before before state
+ * @param after after state
+ * @return {@code true} if there are modifications, {@code false} otherwise
+ */
+ public static boolean modified(NodeState before, NodeState after) {
+ return !after.compareAgainstBaseState(before, new EqualsDiff() {
+ @Override
+ public boolean childNodeChanged(
+ String name, NodeState before, NodeState after) {
+ return true;
+ }
+ });
+ }
+
+ @Override
+ public boolean propertyAdded(PropertyState after) {
+ return false;
+ }
+
+ @Override
+ public boolean propertyChanged(PropertyState before, PropertyState after) {
+ return false;
+ }
+
+ @Override
+ public boolean propertyDeleted(PropertyState before) {
+ return false;
+ }
+
+ @Override
+ public boolean childNodeAdded(String name, NodeState after) {
+ return false;
+ }
+
+ @Override
+ public boolean childNodeChanged(
+ String name, NodeState before, NodeState after) {
+ return after.compareAgainstBaseState(before, this);
+ }
+
+ @Override
+ public boolean childNodeDeleted(String name, NodeState before) {
+ return false;
+ }
+
+}
Re: svn commit: r1558270 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
core/AbstractRoot.java plugins/memory/MutableNodeState.java spi/state/EqualsDiff.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Jan 15, 2014 at 9:44 AM, Jukka Zitting <ju...@gmail.com> wrote:
> Hmm, you're right. The lock manager re-resolves the path to the node,
> and receives a node instance that no longer shows up as modified.
> Investigating...
Fixed in r1558442.
BR,
Jukka Zitting
Re: svn commit: r1558270 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
core/AbstractRoot.java plugins/memory/MutableNodeState.java spi/state/EqualsDiff.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Jan 15, 2014 at 9:30 AM, Michael Dürig <md...@apache.org> wrote:
> On 15.1.14 2:44 , Jukka Zitting wrote:
>> It looks like the root cause of the failure is in the locking
>> implementation and was just triggered by r1558270, so I'll mark it as
>> a known issue for now and follow up in OAK-150.
>
> I don't think this is a locking problem since it only occurs with the
> SegmentMK.
> The test fails since Node.getStatus() returns EXISTING for a modified node
> [1]. Not sure why the returned status is still wrong after Jukka's recent
> fix though.
Hmm, you're right. The lock manager re-resolves the path to the node,
and receives a node instance that no longer shows up as modified.
Investigating...
BR,
Jukka Zitting
Re: svn commit: r1558270 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
core/AbstractRoot.java plugins/memory/MutableNodeState.java spi/state/EqualsDiff.java
Posted by Michael Dürig <md...@apache.org>.
On 15.1.14 2:44 , Jukka Zitting wrote:
> Hi,
>
> On Wed, Jan 15, 2014 at 7:46 AM, Marcel Reutegger <mr...@adobe.com> wrote:
>> this change causes a test failure:
>>
>> Failed tests: testLockWithPendingChanges(org.apache.jackrabbit.test.api.lock.LockManagerTest): Attempt to lock a node with transient modifications must throw InvalidItemStateException.
>
> Oops, sorry about that. I failed to run the build with
> -PintegrationTesting before committing.
In fact before I asked Jukka to take a look at this I added
MutableTreeTest#modifiedAfterRebase since I thought it demonstrates the
root cause for testLockWithPendingChanges failing. As it seems the
modifiedAfterRebase test doesn't catch the whole problem since with
Jukka's fix this test passes while testLockWithPendingChanges still fails.
>
> It looks like the root cause of the failure is in the locking
> implementation and was just triggered by r1558270, so I'll mark it as
> a known issue for now and follow up in OAK-150.
I don't think this is a locking problem since it only occurs with the
SegmentMK.
The test fails since Node.getStatus() returns EXISTING for a modified
node [1]. Not sure why the returned status is still wrong after Jukka's
recent fix though.
Michael
[1]
https://github.com/apache/jackrabbit-oak/blob/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/lock/LockManagerImpl.java#L146-146
>
> BR,
>
> Jukka Zitting
>
Re: svn commit: r1558270 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
core/AbstractRoot.java plugins/memory/MutableNodeState.java spi/state/EqualsDiff.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Jan 15, 2014 at 7:46 AM, Marcel Reutegger <mr...@adobe.com> wrote:
> this change causes a test failure:
>
> Failed tests: testLockWithPendingChanges(org.apache.jackrabbit.test.api.lock.LockManagerTest): Attempt to lock a node with transient modifications must throw InvalidItemStateException.
Oops, sorry about that. I failed to run the build with
-PintegrationTesting before committing.
It looks like the root cause of the failure is in the locking
implementation and was just triggered by r1558270, so I'll mark it as
a known issue for now and follow up in OAK-150.
BR,
Jukka Zitting
RE: svn commit: r1558270 - in
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak:
core/AbstractRoot.java plugins/memory/MutableNodeState.java
spi/state/EqualsDiff.java
Posted by Marcel Reutegger <mr...@adobe.com>.
Hi,
this change causes a test failure:
Failed tests: testLockWithPendingChanges(org.apache.jackrabbit.test.api.lock.LockManagerTest): Attempt to lock a node with transient modifications must throw InvalidItemStateException.
see also travis build:
https://travis-ci.org/apache/jackrabbit-oak/builds/16972897
regards
marcel
> -----Original Message-----
> From: jukka@apache.org [mailto:jukka@apache.org]
> Sent: Mittwoch, 15. Januar 2014 03:49
> To: oak-commits@jackrabbit.apache.org
> Subject: svn commit: r1558270 - in /jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak: core/AbstractRoot.java
> plugins/memory/MutableNodeState.java spi/state/EqualsDiff.java
>
> Author: jukka
> Date: Wed Jan 15 02:48:59 2014
> New Revision: 1558270
>
> URL: http://svn.apache.org/r1558270
> Log:
> OAK-659: Move purge logic for transient changes below the NodeBuilder
> interface
>
> Remove the extra equals() check from AbstractRoot.rebase().
> Fix the MutableNodeState.isModified() check to correctly handle the case
> where the modifications have been flushed.
>
> Added:
> jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java
> Modified:
> jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
> jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNo
> deState.java
>
> Modified: jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
> URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java?rev=
> 1558270&r1=1558269&r2=1558270&view=diff
> ==========================================================
> ====================
> --- jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
> (original)
> +++ jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java Wed
> Jan 15 02:48:59 2014
> @@ -237,12 +237,10 @@ public abstract class AbstractRoot imple
> @Override
> public void rebase() {
> checkLive();
> - if (!store.getRoot().equals(getBaseState())) { // TODO: do we need this?
> - store.rebase(builder);
> - secureBuilder.baseChanged();
> - if (permissionProvider.hasValue()) {
> - permissionProvider.get().refresh();
> - }
> + store.rebase(builder);
> + secureBuilder.baseChanged();
> + if (permissionProvider.hasValue()) {
> + permissionProvider.get().refresh();
> }
> }
>
>
> Modified: jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNo
> deState.java
> URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNo
> deState.java?rev=1558270&r1=1558269&r2=1558270&view=diff
> ==========================================================
> ====================
> --- jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNo
> deState.java (original)
> +++ jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MutableNo
> deState.java Wed Jan 15 02:48:59 2014
> @@ -31,6 +31,7 @@ import javax.annotation.Nonnull;
> import org.apache.jackrabbit.oak.api.PropertyState;
> import org.apache.jackrabbit.oak.spi.state.AbstractNodeState;
> import org.apache.jackrabbit.oak.spi.state.ChildNodeEntry;
> +import org.apache.jackrabbit.oak.spi.state.EqualsDiff;
> import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
> import org.apache.jackrabbit.oak.spi.state.NodeState;
> import org.apache.jackrabbit.oak.spi.state.NodeStateDiff;
> @@ -120,7 +121,7 @@ class MutableNodeState extends AbstractN
> if (!exists()) {
> return false;
> } else if (nodes.isEmpty() && properties.isEmpty()) {
> - return false;
> + return EqualsDiff.modified(before, base);
> }
>
> // was a child node added or removed?
>
> Added: jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java
> URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java?rev
> =1558270&view=auto
> ==========================================================
> ====================
> --- jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java
> (added)
> +++ jackrabbit/oak/trunk/oak-
> core/src/main/java/org/apache/jackrabbit/oak/spi/state/EqualsDiff.java
> Wed Jan 15 02:48:59 2014
> @@ -0,0 +1,91 @@
> +/*
> + * Licensed to the Apache Software Foundation (ASF) under one
> + * or more contributor license agreements. See the NOTICE file
> + * distributed with this work for additional information
> + * regarding copyright ownership. The ASF licenses this file
> + * to you under the Apache License, Version 2.0 (the
> + * "License"); you may not use this file except in compliance
> + * with the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> + * software distributed under the License is distributed on an
> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + * KIND, either express or implied. See the License for the
> + * specific language governing permissions and limitations
> + * under the License.
> + */
> +package org.apache.jackrabbit.oak.spi.state;
> +
> +import org.apache.jackrabbit.oak.api.PropertyState;
> +
> +/**
> + * Helper class for comparing the equality of node states based on the
> + * content diff mechanism.
> + */
> +public class EqualsDiff implements NodeStateDiff {
> +
> + /**
> + * Diffs the given node states and returns {@code true} if there are no
> + * differences.
> + *
> + * @param before before state
> + * @param after after state
> + * @return {@code true} if the states are equal, {@code false} otherwise
> + */
> + public static boolean equals(NodeState before, NodeState after) {
> + return before.exists() == after.exists()
> + && after.compareAgainstBaseState(before, new EqualsDiff());
> + }
> +
> + /**
> + * Diffs the given node states and returns {@code true} if there are
> + * differences within the properties or direct child nodes.
> + *
> + * @param before before state
> + * @param after after state
> + * @return {@code true} if there are modifications, {@code false}
> otherwise
> + */
> + public static boolean modified(NodeState before, NodeState after) {
> + return !after.compareAgainstBaseState(before, new EqualsDiff() {
> + @Override
> + public boolean childNodeChanged(
> + String name, NodeState before, NodeState after) {
> + return true;
> + }
> + });
> + }
> +
> + @Override
> + public boolean propertyAdded(PropertyState after) {
> + return false;
> + }
> +
> + @Override
> + public boolean propertyChanged(PropertyState before, PropertyState
> after) {
> + return false;
> + }
> +
> + @Override
> + public boolean propertyDeleted(PropertyState before) {
> + return false;
> + }
> +
> + @Override
> + public boolean childNodeAdded(String name, NodeState after) {
> + return false;
> + }
> +
> + @Override
> + public boolean childNodeChanged(
> + String name, NodeState before, NodeState after) {
> + return after.compareAgainstBaseState(before, this);
> + }
> +
> + @Override
> + public boolean childNodeDeleted(String name, NodeState before) {
> + return false;
> + }
> +
> +}
>