Author: mreutegg
Date: Mon Jul 2 07:30:20 2018
New Revision: 1834823
URL: http://svn.apache.org/viewvc?rev=1834823&view=rev
Log:
OAK-7593: NodeDocument.getLatestValue() may throw IllegalStateException
Modified:
jackrabbit/oak/trunk/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java
jackrabbit/oak/trunk/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
Modified:
jackrabbit/oak/trunk/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java?rev=1834823&r1=1834822&r2=1834823&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java
(original)
+++
jackrabbit/oak/trunk/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java
Mon Jul 2 07:30:20 2018
@@ -51,7 +51,6 @@ import org.apache.jackrabbit.oak.commons
import org.apache.jackrabbit.oak.commons.json.JsopTokenizer;
import org.apache.jackrabbit.oak.commons.json.JsopWriter;
import org.apache.jackrabbit.oak.plugins.document.memory.MemoryDocumentStore;
-import org.apache.jackrabbit.oak.plugins.document.util.MergeSortedIterators;
import org.apache.jackrabbit.oak.plugins.document.util.Utils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -64,7 +63,6 @@ import static com.google.common.base.Obj
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.collect.ImmutableList.copyOf;
-import static com.google.common.collect.Iterables.concat;
import static com.google.common.collect.Iterables.filter;
import static com.google.common.collect.Iterables.mergeSorted;
import static com.google.common.collect.Iterables.transform;
@@ -1537,38 +1535,40 @@ public final class NodeDocument extends
@Nonnull final Revision
readRevision,
@Nonnull final
List<Iterable<Entry<Revision, String>>> changes) {
List<Iterable<Map.Entry<Revision, String>>> revs =
Lists.newArrayList();
- Revision lowRev = new Revision(Long.MAX_VALUE, 0,
readRevision.getClusterId());
RevisionVector readRV = new RevisionVector(readRevision);
- List<Range> ranges = Lists.newArrayList();
- for (Map.Entry<Revision, Range> e : getPreviousRanges().entrySet()) {
- Range range = e.getValue();
- if (range.low.getClusterId() != readRevision.getClusterId()
- || readRevision.compareRevisionTime(range.low) < 0) {
- // either clusterId does not match
- // or range is newer than read revision
- continue;
- }
- // check if it overlaps with previous ranges
- if (range.high.compareRevisionTime(lowRev) < 0) {
- // does not overlap
- if (!ranges.isEmpty()) {
- // there are previous ranges
- // get and merge sort overlapping ranges
- revs.add(changesFor(ranges, readRV, property));
- ranges.clear();
- }
- }
- ranges.add(range);
- lowRev = Utils.min(lowRev, range.low);
- }
- if (!ranges.isEmpty()) {
- // get remaining changes
- revs.add(changesFor(ranges, readRV, property));
- }
-
- if (!revs.isEmpty()) {
- changes.add(concat(revs));
+ List<Range> ranges = new ArrayList<>();
+ for (Range r : getPreviousRanges().values()) {
+ if (r.low.getClusterId() == readRevision.getClusterId()
+ && readRevision.compareRevisionTime(r.low) >= 0) {
+ // clusterId matches and range is visible from read revision
+ ranges.add(r);
+ }
+ }
+ List<Range> batch = new ArrayList<>();
+ while (!ranges.isEmpty()) {
+ // create batches of non-overlapping ranges
+ Range previous = null;
+ Iterator<Range> it = ranges.iterator();
+ while (it.hasNext()) {
+ Range r = it.next();
+ if (previous == null ||
r.high.compareRevisionTime(previous.low) < 0) {
+ // first range or does not overlap with previous in batch
+ batch.add(r);
+ it.remove();
+ previous = r;
+ }
+ }
+ revs.add(changesFor(batch, readRV, property));
+ batch.clear();
+ }
+
+ if (revs.size() == 1) {
+ // optimize single batch case
+ changes.add(revs.get(0));
+ } else if (!revs.isEmpty()) {
+ // merge sort them
+ changes.add(mergeSorted(revs, ValueComparator.REVERSE));
}
}
@@ -1611,34 +1611,7 @@ public final class NodeDocument extends
}
};
} else {
- changes = new Iterable<Entry<Revision, String>>() {
- private List<Range> rangeList = copyOf(ranges);
- private Iterable<Iterable<Entry<Revision, String>>>
changesPerRange
- = transform(rangeList, rangeToChanges);
- @Override
- public Iterator<Entry<Revision, String>> iterator() {
- final Iterator<Iterable<Entry<Revision, String>>> it
- = checkNotNull(changesPerRange.iterator());
- return new MergeSortedIterators<Entry<Revision,
String>>(ValueComparator.REVERSE) {
- @Override
- public Iterator<Entry<Revision, String>>
nextIterator() {
- while (it.hasNext()) {
- Iterator<Entry<Revision, String>> next =
it.next().iterator();
- // check if this even has elements
- if (next.hasNext()) {
- return next;
- }
- }
- return null;
- }
-
- @Override
- public String description() {
- return "Ranges to merge sort: " + rangeList;
- }
- };
- }
- };
+ changes = Iterables.concat(transform(copyOf(ranges),
rangeToChanges));
}
return filter(changes, new Predicate<Entry<Revision, String>>() {
@Override
Modified:
jackrabbit/oak/trunk/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java?rev=1834823&r1=1834822&r2=1834823&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
(original)
+++
jackrabbit/oak/trunk/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentTest.java
Mon Jul 2 07:30:20 2018
@@ -39,7 +39,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.Maps.newLinkedHashMap;
@@ -756,7 +755,7 @@ public class NodeDocumentTest {
assertEquals(String.valueOf(numChanges - (i + 1)), value);
assertTrue("too many calls for previous documents ("
+ prevDocCalls.size() + "): " + prevDocCalls,
- prevDocCalls.size() <= 8);
+ prevDocCalls.size() <= 10);
}
ns1.dispose();
@@ -781,7 +780,8 @@ public class NodeDocumentTest {
DocumentNodeStore ns = createTestStore(store, 1, 0);
List<RevisionVector> headRevisions = Lists.newArrayList();
- for (int i = 0; i < 1000; i++) {
+ long count = 1000;
+ for (int i = 0; i < count; i++) {
NodeBuilder builder = ns.getRoot().builder();
NodeBuilder test = builder.child("test");
test.setProperty("p", i);
@@ -807,17 +807,18 @@ public class NodeDocumentTest {
ns.runBackgroundOperations();
NodeDocument doc = store.find(NODES, Utils.getIdFromPath("/test"));
+ assertNotNull(doc);
List<Integer> numCalls = Lists.newArrayList();
- long count = 0;
// go back in time and check number of find calls
+ Collections.reverse(headRevisions);
for (RevisionVector rv : headRevisions) {
prevDocCalls.clear();
DocumentNodeState s = doc.getNodeAtRevision(ns, rv, null);
assertNotNull(s);
- assertEquals(count++,
s.getProperty("p").getValue(Type.LONG).longValue());
+ assertEquals(--count,
s.getProperty("p").getValue(Type.LONG).longValue());
numCalls.add(prevDocCalls.size());
}
- assertThat(numCalls.toString(), numCalls, everyItem(lessThan(36)));
+ assertThat(numCalls.toString(), numCalls, everyItem(lessThan(43)));
ns.dispose();
}
@@ -864,7 +865,6 @@ public class NodeDocumentTest {
}
// OAK-7593
- @Ignore("OAK-7593")
@Test
public void getVisibleChangesWithOverlappingRanges() throws Exception {
MemoryDocumentStore store = new MemoryDocumentStore();
