Author: mreutegg Date: Mon May 19 12:49:24 2014 New Revision: 1595875 URL: http://svn.apache.org/r1595875 Log: OAK-1822: NodeDocument _modified may go back in time
Added: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateOp.java jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateUtils.java jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/AbstractDocumentStoreTest.java jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java?rev=1595875&r1=1595874&r2=1595875&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java (original) +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java Mon May 19 12:49:24 2014 @@ -1205,7 +1205,7 @@ public final class NodeDocument extends public static void setModified(@Nonnull UpdateOp op, @Nonnull Revision revision) { - checkNotNull(op).set(MODIFIED_IN_SECS, getModifiedInSecs(checkNotNull(revision).getTimestamp())); + checkNotNull(op).max(MODIFIED_IN_SECS, getModifiedInSecs(checkNotNull(revision).getTimestamp())); } public static void setRevision(@Nonnull UpdateOp op, Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateOp.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateOp.java?rev=1595875&r1=1595874&r2=1595875&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateOp.java (original) +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateOp.java Mon May 19 12:49:24 2014 @@ -131,9 +131,7 @@ public final class UpdateOp { * @param value the value */ void setMapEntry(@Nonnull String property, @Nonnull Revision revision, Object value) { - Operation op = new Operation(); - op.type = Operation.Type.SET_MAP_ENTRY; - op.value = value; + Operation op = new Operation(Operation.Type.SET_MAP_ENTRY, value); changes.put(new Key(property, checkNotNull(revision)), op); } @@ -145,8 +143,7 @@ public final class UpdateOp { * @param revision the revision */ public void removeMapEntry(@Nonnull String property, @Nonnull Revision revision) { - Operation op = new Operation(); - op.type = Operation.Type.REMOVE_MAP_ENTRY; + Operation op = new Operation(Operation.Type.REMOVE_MAP_ENTRY, null); changes.put(new Key(property, checkNotNull(revision)), op); } @@ -157,9 +154,23 @@ public final class UpdateOp { * @param value the value */ void set(String property, Object value) { - Operation op = new Operation(); - op.type = Operation.Type.SET; - op.value = value; + Operation op = new Operation(Operation.Type.SET, value); + changes.put(new Key(property, null), op); + } + + /** + * Set the property to the given value if the new value is higher than the + * existing value. The property is also set to the given value if the + * property does not yet exist. + * <p> + * The result of a max operation with different types of values is + * undefined. + * + * @param property the name of the property to set. + * @param value the new value for the property. + */ + <T> void max(String property, Comparable<T> value) { + Operation op = new Operation(Operation.Type.MAX, value); changes.put(new Key(property, null), op); } @@ -187,9 +198,7 @@ public final class UpdateOp { if (isNew) { throw new IllegalStateException("Cannot use containsMapEntry() on new document"); } - Operation op = new Operation(); - op.type = Operation.Type.CONTAINS_MAP_ENTRY; - op.value = exists; + Operation op = new Operation(Operation.Type.CONTAINS_MAP_ENTRY, exists); changes.put(new Key(property, checkNotNull(revision)), op); } @@ -200,9 +209,7 @@ public final class UpdateOp { * @param value the increment */ public void increment(@Nonnull String property, long value) { - Operation op = new Operation(); - op.type = Operation.Type.INCREMENT; - op.value = value; + Operation op = new Operation(Operation.Type.INCREMENT, value); changes.put(new Key(property, null), op); } @@ -239,6 +246,14 @@ public final class UpdateOp { SET, /** + * Set the value if the new value is higher than the existing value. + * The new value is also considered higher, when there is no + * existing value. + * The sub-key is not used. + */ + MAX, + + /** * Increment the Long value with the provided Long value. * The sub-key is not used. */ @@ -267,12 +282,17 @@ public final class UpdateOp { /** * The operation type. */ - public Type type; + public final Type type; /** * The value, if any. */ - public Object value; + public final Object value; + + Operation(Type type, Object value) { + this.type = checkNotNull(type); + this.value = value; + } @Override public String toString() { @@ -283,18 +303,16 @@ public final class UpdateOp { Operation reverse = null; switch (type) { case INCREMENT: - reverse = new Operation(); - reverse.type = Type.INCREMENT; - reverse.value = -(Long) value; + reverse = new Operation(Type.INCREMENT, -(Long) value); break; case SET: + case MAX: case REMOVE_MAP_ENTRY: case CONTAINS_MAP_ENTRY: // nothing to do break; case SET_MAP_ENTRY: - reverse = new Operation(); - reverse.type = Type.REMOVE_MAP_ENTRY; + reverse = new Operation(Type.REMOVE_MAP_ENTRY, null); break; } return reverse; Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateUtils.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateUtils.java?rev=1595875&r1=1595874&r2=1595875&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateUtils.java (original) +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateUtils.java Mon May 19 12:49:24 2014 @@ -44,7 +44,9 @@ public class UpdateUtils { * @param comparator * the revision comparator. */ - public static void applyChanges(@Nonnull Document doc, @Nonnull UpdateOp update, @Nonnull Comparator<Revision> comparator) { + public static void applyChanges(@Nonnull Document doc, + @Nonnull UpdateOp update, + @Nonnull Comparator<Revision> comparator) { for (Entry<Key, Operation> e : checkNotNull(update).getChanges().entrySet()) { Key k = e.getKey(); Operation op = e.getValue(); @@ -53,6 +55,15 @@ public class UpdateUtils { doc.put(k.toString(), op.value); break; } + case MAX: { + Comparable newValue = (Comparable) op.value; + Object old = doc.get(k.toString()); + //noinspection unchecked + if (old == null || newValue.compareTo(old) > 0) { + doc.put(k.toString(), op.value); + } + break; + } case INCREMENT: { Object old = doc.get(k.toString()); Long x = (Long) op.value; Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java?rev=1595875&r1=1595874&r2=1595875&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java (original) +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java Mon May 19 12:49:24 2014 @@ -570,6 +570,7 @@ public class MongoDocumentStore implemen Operation op = entry.getValue(); switch (op.type) { case SET: + case MAX: case INCREMENT: { inserts[i].put(k.toString(), op.value); break; @@ -965,6 +966,7 @@ public class MongoDocumentStore implemen @Nonnull private static DBObject createUpdate(UpdateOp updateOp) { BasicDBObject setUpdates = new BasicDBObject(); + BasicDBObject maxUpdates = new BasicDBObject(); BasicDBObject incUpdates = new BasicDBObject(); BasicDBObject unsetUpdates = new BasicDBObject(); @@ -980,16 +982,17 @@ public class MongoDocumentStore implemen } Operation op = entry.getValue(); switch (op.type) { - case SET: { + case SET: + case SET_MAP_ENTRY: { setUpdates.append(k.toString(), op.value); break; } - case INCREMENT: { - incUpdates.append(k.toString(), op.value); + case MAX: { + maxUpdates.append(k.toString(), op.value); break; } - case SET_MAP_ENTRY: { - setUpdates.append(k.toString(), op.value); + case INCREMENT: { + incUpdates.append(k.toString(), op.value); break; } case REMOVE_MAP_ENTRY: { @@ -1003,6 +1006,9 @@ public class MongoDocumentStore implemen if (!setUpdates.isEmpty()) { update.append("$set", setUpdates); } + if (!maxUpdates.isEmpty()) { + update.append("$max", maxUpdates); + } if (!incUpdates.isEmpty()) { update.append("$inc", incUpdates); } Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/AbstractDocumentStoreTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/AbstractDocumentStoreTest.java?rev=1595875&r1=1595874&r2=1595875&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/AbstractDocumentStoreTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/AbstractDocumentStoreTest.java Mon May 19 12:49:24 2014 @@ -34,17 +34,19 @@ public abstract class AbstractDocumentSt protected String dsname; protected DocumentStore ds; + protected DocumentStoreFixture dsf; protected List<String> removeMe = new ArrayList<String>(); static final Logger LOG = LoggerFactory.getLogger(AbstractDocumentStoreTest.class); public AbstractDocumentStoreTest(DocumentStoreFixture dsf) { + this.dsf = dsf; this.ds = dsf.createDocumentStore(); this.dsname = dsf.getName(); } @After - public void cleanUp() { + public void cleanUp() throws Exception { if (!removeMe.isEmpty()) { long start = System.nanoTime(); try { @@ -65,6 +67,7 @@ public abstract class AbstractDocumentSt LOG.info(removeMe.size() + " documents removed in " + elapsed + "ms (" + rate + "/ms)"); } } + dsf.dispose(); } @Parameterized.Parameters Added: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java?rev=1595875&view=auto ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java (added) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java Mon May 19 12:49:24 2014 @@ -0,0 +1,116 @@ +/* + * 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.plugins.document; + +import org.apache.jackrabbit.oak.plugins.document.util.TimingDocumentStoreWrapper; +import org.apache.jackrabbit.oak.spi.commit.CommitInfo; +import org.apache.jackrabbit.oak.spi.commit.EmptyHook; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.apache.jackrabbit.oak.stats.Clock; +import org.junit.After; +import org.junit.Test; + +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.apache.jackrabbit.oak.plugins.document.NodeDocument.MODIFIED_IN_SECS_RESOLUTION; +import static org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath; +import static org.junit.Assert.assertEquals; + +/** + * Tests DocumentNodeStore on various DocumentStore back-ends. + */ +public class DocumentNodeStoreIT extends AbstractDocumentStoreTest { + + + public DocumentNodeStoreIT(DocumentStoreFixture dsf) { + super(dsf); + } + + @After + public void tearDown() { + Revision.resetClockToDefault(); + } + + + @Test + public void modifiedResetWithDiff() throws Exception { + Clock clock = new Clock.Virtual(); + clock.waitUntil(System.currentTimeMillis()); + Revision.setClock(clock); + DocumentStore docStore = new TimingDocumentStoreWrapper(ds) { + @Override + public void dispose() { + // do not dispose yet + } + }; + DocumentNodeStore ns1 = new DocumentMK.Builder() + .setDocumentStore(docStore).setClusterId(1) + .setAsyncDelay(0).clock(clock) + // use a no-op diff cache to simulate a cache miss + // when the diff is made later in the test + .setDiffCache(AmnesiaDiffCache.INSTANCE) + .getNodeStore(); + NodeBuilder builder1 = ns1.getRoot().builder(); + builder1.child("node"); + removeMe.add(getIdFromPath("/node")); + for (int i = 0; i < DocumentMK.MANY_CHILDREN_THRESHOLD; i++) { + builder1.child("node-" + i); + removeMe.add(getIdFromPath("/node/node-" + i)); + } + ns1.merge(builder1, EmptyHook.INSTANCE, CommitInfo.EMPTY); + // make sure commit is visible to other node store instance + ns1.runBackgroundOperations(); + + DocumentNodeStore ns2 = new DocumentMK.Builder() + .setDocumentStore(docStore).setClusterId(2) + .setAsyncDelay(0).clock(clock).getNodeStore(); + + NodeBuilder builder2 = ns2.getRoot().builder(); + builder2.child("node").child("child-a"); + removeMe.add(getIdFromPath("/node/child-a")); + ns2.merge(builder2, EmptyHook.INSTANCE, CommitInfo.EMPTY); + + // wait at least _modified resolution. in reality the wait may + // not be necessary. e.g. when the clock passes the resolution boundary + // exactly at this time + clock.waitUntil(System.currentTimeMillis() + + SECONDS.toMillis(MODIFIED_IN_SECS_RESOLUTION + 1)); + + builder1 = ns1.getRoot().builder(); + builder1.child("node").child("child-b"); + removeMe.add(getIdFromPath("/node/child-b")); + ns1.merge(builder1, EmptyHook.INSTANCE, CommitInfo.EMPTY); + // remember root for diff + DocumentNodeState root1 = ns1.getRoot(); + + builder1 = root1.builder(); + builder1.child("node").child("child-c"); + removeMe.add(getIdFromPath("/node/child-c")); + ns1.merge(builder1, EmptyHook.INSTANCE, CommitInfo.EMPTY); + // remember root for diff + DocumentNodeState root2 = ns1.getRoot(); + + ns1.runBackgroundOperations(); + ns2.runBackgroundOperations(); + + String diff = ns1.diffChildren(root2, root1); + // must report /node as changed + assertEquals("^\"node\":{}", diff.trim()); + + ns1.dispose(); + ns2.dispose(); + } +} Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java?rev=1595875&r1=1595874&r2=1595875&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java Mon May 19 12:49:24 2014 @@ -51,7 +51,6 @@ import org.apache.jackrabbit.oak.spi.sta import org.apache.jackrabbit.oak.spi.state.NodeState; import org.apache.jackrabbit.oak.stats.Clock; import org.junit.After; -import org.junit.Ignore; import org.junit.Test; import com.google.common.collect.Iterables; @@ -419,7 +418,6 @@ public class DocumentNodeStoreTest { ns.dispose(); } - @Ignore("OAK-1822") @Test public void modifiedReset() throws Exception { Clock clock = new Clock.Virtual(); @@ -469,66 +467,6 @@ public class DocumentNodeStoreTest { ns2.dispose(); } - @Ignore("OAK-1822") - @Test - public void modifiedResetWithDiff() throws Exception { - Clock clock = new Clock.Virtual(); - clock.waitUntil(System.currentTimeMillis()); - Revision.setClock(clock); - MemoryDocumentStore docStore = new MemoryDocumentStore(); - DocumentNodeStore ns1 = new DocumentMK.Builder() - .setDocumentStore(docStore).setClusterId(1) - .setAsyncDelay(0).clock(clock) - // use a no-op diff cache to simulate a cache miss - // when the diff is made later in the test - .setDiffCache(AmnesiaDiffCache.INSTANCE) - .getNodeStore(); - NodeBuilder builder1 = ns1.getRoot().builder(); - builder1.child("node"); - for (int i = 0; i < DocumentMK.MANY_CHILDREN_THRESHOLD; i++) { - builder1.child("node-" + i); - } - ns1.merge(builder1, EmptyHook.INSTANCE, CommitInfo.EMPTY); - // make sure commit is visible to other node store instance - ns1.runBackgroundOperations(); - - DocumentNodeStore ns2 = new DocumentMK.Builder() - .setDocumentStore(docStore).setClusterId(2) - .setAsyncDelay(0).clock(clock).getNodeStore(); - - NodeBuilder builder2 = ns2.getRoot().builder(); - builder2.child("node").child("child-a"); - ns2.merge(builder2, EmptyHook.INSTANCE, CommitInfo.EMPTY); - - // wait at least _modified resolution. in reality the wait may - // not be necessary. e.g. when the clock passes the resolution boundary - // exactly at this time - clock.waitUntil(System.currentTimeMillis() + - SECONDS.toMillis(MODIFIED_IN_SECS_RESOLUTION + 1)); - - builder1 = ns1.getRoot().builder(); - builder1.child("node").child("child-b"); - ns1.merge(builder1, EmptyHook.INSTANCE, CommitInfo.EMPTY); - // remember root for diff - DocumentNodeState root1 = ns1.getRoot(); - - builder1 = root1.builder(); - builder1.child("node").child("child-c"); - ns1.merge(builder1, EmptyHook.INSTANCE, CommitInfo.EMPTY); - // remember root for diff - DocumentNodeState root2 = ns1.getRoot(); - - ns1.runBackgroundOperations(); - ns2.runBackgroundOperations(); - - String diff = ns1.diffChildren(root2, root1); - // must report /node as changed - assertEquals("^\"node\":{}", diff); - - ns1.dispose(); - ns2.dispose(); - } - private static class TestHook extends EditorHook { TestHook(final String prefix) {