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 mr...@apache.org on 2016/05/31 21:04:24 UTC
svn commit: r1746345 - in /jackrabbit/oak/trunk/oak-core/src:
main/java/org/apache/jackrabbit/oak/plugins/document/
main/java/org/apache/jackrabbit/oak/plugins/document/util/
test/java/org/apache/jackrabbit/oak/plugins/document/
Author: mreutegg
Date: Tue May 31 21:04:24 2016
New Revision: 1746345
URL: http://svn.apache.org/viewvc?rev=1746345&view=rev
Log:
OAK-4358: Stale cluster ids can potentially lead to lots of previous docs traversal in NodeDocument.getNewestRevision
Implement fix and enable test
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/util/Utils.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.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=1746345&r1=1746344&r2=1746345&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 Tue May 31 21:04:24 2016
@@ -19,6 +19,7 @@ package org.apache.jackrabbit.oak.plugin
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
+import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.NavigableMap;
@@ -39,6 +40,7 @@ import com.google.common.base.Predicate;
import com.google.common.collect.AbstractIterator;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterators;
+import com.google.common.collect.Lists;
import com.google.common.collect.Queues;
import org.apache.jackrabbit.oak.cache.CacheValue;
import org.apache.jackrabbit.oak.commons.PathUtils;
@@ -54,7 +56,6 @@ import org.slf4j.LoggerFactory;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
-import com.google.common.primitives.Longs;
import static com.google.common.base.Objects.equal;
import static com.google.common.base.Preconditions.checkArgument;
@@ -65,6 +66,7 @@ import static org.apache.jackrabbit.oak.
import static org.apache.jackrabbit.oak.plugins.document.StableRevisionComparator.REVERSE;
import static org.apache.jackrabbit.oak.plugins.document.UpdateOp.Key;
import static org.apache.jackrabbit.oak.plugins.document.UpdateOp.Operation;
+import static org.apache.jackrabbit.oak.plugins.document.util.Utils.abortingIterable;
import static org.apache.jackrabbit.oak.plugins.document.util.Utils.resolveCommitRevision;
/**
@@ -759,18 +761,24 @@ public final class NodeDocument extends
}
// if we don't have clusterIds, we can use the local changes only
boolean fullScan = true;
- Iterable<Revision> changes;
- if (clusterIds.isEmpty()) {
- // baseRev is newer than all previous documents
- changes = Iterables.mergeSorted(
- ImmutableList.of(
- getLocalRevisions().keySet(),
- getLocalCommitRoot().keySet()),
- getLocalRevisions().comparator());
- } else {
+ Iterable<Revision> changes = Iterables.mergeSorted(
+ ImmutableList.of(
+ getLocalRevisions().keySet(),
+ getLocalCommitRoot().keySet()),
+ getLocalRevisions().comparator()
+ );
+ if (!clusterIds.isEmpty()) {
+ // there are some previous documents that potentially
+ // contain changes after 'lower' revision vector
// include previous documents as well (only needed in rare cases)
fullScan = false;
- changes = getAllChanges();
+ changes = Iterables.mergeSorted(
+ ImmutableList.of(
+ changes,
+ getChanges(REVISIONS, lower),
+ getChanges(COMMIT_ROOT, lower)
+ ), getLocalRevisions().comparator()
+ );
if (LOG.isDebugEnabled()) {
LOG.debug("getNewestRevision() with changeRev {} on {}, " +
"_revisions {}, _commitRoot {}",
@@ -1453,90 +1461,18 @@ public final class NodeDocument extends
* @return revisions of all changes performed on this document.
*/
Iterable<Revision> getAllChanges() {
- final SortedSet<Revision> stack = Sets.newTreeSet(REVERSE);
- // initialize with local revisions and commitRoot entries
- stack.addAll(getLocalCommitRoot().keySet());
- stack.addAll(getLocalRevisions().keySet());
- if (getPreviousRanges().isEmpty()) {
- return stack;
- }
- return new Iterable<Revision>() {
- @Override
- public Iterator<Revision> iterator() {
- final Iterator<NodeDocument> previousDocs = getPreviousDocLeaves();
- return new AbstractIterator<Revision>() {
- private NodeDocument nextDoc;
- private Revision nextRevision;
- @Override
- protected Revision computeNext() {
- if (stack.isEmpty()) {
- return endOfData();
- }
- Revision next = stack.first();
- stack.remove(next);
- fillStackIfNeeded();
- return next;
- }
-
- private void fillStackIfNeeded() {
- for (;;) {
- fetchNextDoc();
-
- // no more changes to compare with
- if (nextDoc == null) {
- return;
- }
-
- // check if current top revision is still newer than
- // most recent revision of next document
- if (!stack.isEmpty()) {
- Revision top = stack.first();
- if (top.compareRevisionTimeThenClusterId(nextRevision) > 0) {
- return;
- }
- }
-
- // if we get here, we need to pull in changes
- // from nextDoc
- Iterables.addAll(stack, nextDoc.getAllChanges());
- nextDoc = null;
- nextRevision = null;
- }
- }
-
- /**
- * Fetch the next document if {@code nextDoc} is
- * {@code null} and there are more documents.
- */
- private void fetchNextDoc() {
- for (;;) {
- if (nextDoc != null) {
- break;
- }
- if (!previousDocs.hasNext()) {
- // no more previous docs
- break;
- }
- nextDoc = previousDocs.next();
- Iterator<Revision> changes = nextDoc.getAllChanges().iterator();
- if (changes.hasNext()) {
- nextRevision = changes.next();
- break;
- } else {
- // empty document, try next
- nextDoc = null;
- }
- }
- }
- };
- }
- };
+ RevisionVector empty = new RevisionVector();
+ return Iterables.mergeSorted(ImmutableList.of(
+ getChanges(REVISIONS, empty),
+ getChanges(COMMIT_ROOT, empty)
+ ), StableRevisionComparator.REVERSE);
}
/**
* Returns all changes for the given property back to {@code min} revision
* (exclusive). The revisions include committed as well as uncommitted
- * changes.
+ * changes. The returned revisions are sorted in reverse order (newest
+ * first).
*
* @param property the name of the property.
* @param min the lower bound revision (exclusive).
@@ -1545,43 +1481,27 @@ public final class NodeDocument extends
@Nonnull
Iterable<Revision> getChanges(@Nonnull final String property,
@Nonnull final RevisionVector min) {
- return new Iterable<Revision>() {
+ Predicate<Revision> p = new Predicate<Revision>() {
@Override
- public Iterator<Revision> iterator() {
- final Set<Revision> changes = getValueMap(property).keySet();
- final Set<Integer> clusterIds = Sets.newHashSet();
- for (Revision r : getLocalMap(property).keySet()) {
- clusterIds.add(r.getClusterId());
- }
- for (Range r : getPreviousRanges().values()) {
- if (min.isRevisionNewer(r.high)) {
- clusterIds.add(r.high.getClusterId());
- }
- }
- final Iterator<Revision> unfiltered = changes.iterator();
- return new AbstractIterator<Revision>() {
- @Override
- protected Revision computeNext() {
- while (unfiltered.hasNext()) {
- Revision next = unfiltered.next();
- if (min.isRevisionNewer(next)) {
- return next;
- } else {
- // further revisions with this clusterId
- // are older than min revision
- clusterIds.remove(next.getClusterId());
- // no more revisions to check
- if (clusterIds.isEmpty()) {
- return endOfData();
- }
- }
- }
- return endOfData();
- }
- };
+ public boolean apply(Revision input) {
+ return min.isRevisionNewer(input);
}
};
-
+ List<Iterable<Revision>> changes = Lists.newArrayList();
+ changes.add(abortingIterable(getLocalMap(property).keySet(), p));
+ for (Map.Entry<Revision, Range> e : getPreviousRanges().entrySet()) {
+ if (min.isRevisionNewer(e.getKey())) {
+ final NodeDocument prev = getPreviousDoc(e.getKey(), e.getValue());
+ if (prev != null) {
+ changes.add(abortingIterable(prev.getValueMap(property).keySet(), p));
+ }
+ }
+ }
+ if (changes.size() == 1) {
+ return changes.get(0);
+ } else {
+ return Iterables.mergeSorted(changes, StableRevisionComparator.REVERSE);
+ }
}
/**
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java?rev=1746345&r1=1746344&r2=1746345&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java Tue May 31 21:04:24 2016
@@ -758,4 +758,36 @@ public class Utils {
}
return min;
}
+
+ /**
+ * Wraps the given iterable and aborts iteration over elements when the
+ * predicate on an element evaluates to {@code false}.
+ *
+ * @param iterable the iterable to wrap.
+ * @param p the predicate.
+ * @return the aborting iterable.
+ */
+ public static <T> Iterable<T> abortingIterable(final Iterable<T> iterable,
+ final Predicate<T> p) {
+ checkNotNull(iterable);
+ checkNotNull(p);
+ return new Iterable<T>() {
+ @Override
+ public Iterator<T> iterator() {
+ final Iterator<T> it = iterable.iterator();
+ return new AbstractIterator<T>() {
+ @Override
+ protected T computeNext() {
+ if (it.hasNext()) {
+ T next = it.next();
+ if (p.apply(next)) {
+ return next;
+ }
+ }
+ return endOfData();
+ }
+ };
+ }
+ };
+ }
}
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java?rev=1746345&r1=1746344&r2=1746345&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java Tue May 31 21:04:24 2016
@@ -37,7 +37,6 @@ import org.apache.jackrabbit.oak.spi.com
import org.apache.jackrabbit.oak.spi.commit.EmptyHook;
import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
import org.apache.jackrabbit.oak.spi.state.NodeStore;
-import org.junit.Ignore;
import org.junit.Test;
import static com.google.common.collect.Sets.newHashSet;
@@ -555,7 +554,6 @@ public class NodeDocumentTest {
}
// OAK-4358
- @Ignore
@Test
public void tooManyReadsOnGetNewestRevision() throws Exception {
final Set<String> prevDocCalls = newHashSet();