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 2013/12/12 16:39:37 UTC
svn commit: r1550438 - in /jackrabbit/oak/trunk/oak-core/src:
main/java/org/apache/jackrabbit/oak/plugins/mongomk/
test/java/org/apache/jackrabbit/oak/plugins/mongomk/
Author: mreutegg
Date: Thu Dec 12 15:39:37 2013
New Revision: 1550438
URL: http://svn.apache.org/r1550438
Log:
OAK-1274: Wrong comparison for old Revisions
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/StableRevisionComparator.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RandomizedClusterTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java?rev=1550438&r1=1550437&r2=1550438&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java Thu Dec 12 15:39:37 2013
@@ -72,18 +72,56 @@ public class Revision {
/**
* Compare the time part of two revisions. If they contain the same time,
* the counter is compared.
+ * <p>
+ * This method requires that both revisions are from the same cluster node.
*
+ * @param other the other revision
* @return -1 if this revision occurred earlier, 1 if later, 0 if equal
+ * @throws IllegalArgumentException if the cluster ids don't match
*/
int compareRevisionTime(Revision other) {
+ if (clusterId != other.clusterId) {
+ throw new IllegalArgumentException(
+ "Trying to compare revisions of different cluster ids: " +
+ this + " and " + other);
+ }
+ int comp = timestamp < other.timestamp ? -1 : timestamp > other.timestamp ? 1 : 0;
+ if (comp == 0) {
+ comp = counter < other.counter ? -1 : counter > other.counter ? 1 : 0;
+ }
+ return comp;
+ }
+
+ /**
+ * Compare the time part of two revisions. If they contain the same time,
+ * the counter is compared. If the counter is the same, the cluster ids are
+ * compared.
+ *
+ * @param other the other revision
+ * @return -1 if this revision occurred earlier, 1 if later, 0 if equal
+ */
+ int compareRevisionTimeThenClusterId(Revision other) {
int comp = timestamp < other.timestamp ? -1 : timestamp > other.timestamp ? 1 : 0;
if (comp == 0) {
comp = counter < other.counter ? -1 : counter > other.counter ? 1 : 0;
}
+ if (comp == 0) {
+ comp = compareClusterId(other);
+ }
return comp;
}
/**
+ * Compare the cluster node ids of both revisions.
+ *
+ * @param other the other revision
+ * @return -1 if this revision occurred earlier, 1 if later, 0 if equal
+ */
+ int compareClusterId(Revision other) {
+ return clusterId < other.clusterId ? -1 : clusterId > other.clusterId ? 1 : 0;
+ }
+
+ /**
* Create a simple revision id. The format is similar to MongoDB ObjectId.
*
* @param clusterId the unique machineId + processId
@@ -298,9 +336,9 @@ public class Revision {
*/
public static class RevisionComparator implements Comparator<Revision> {
- private static final Revision NEWEST = new Revision(Long.MAX_VALUE, 0, 0);
+ static final Revision NEWEST = new Revision(Long.MAX_VALUE, 0, 0);
- private static final Revision FUTURE = new Revision(Long.MAX_VALUE, Integer.MAX_VALUE, 0);
+ static final Revision FUTURE = new Revision(Long.MAX_VALUE, Integer.MAX_VALUE, 0);
/**
* The map of cluster instances to lists of revision ranges.
@@ -427,31 +465,45 @@ public class Revision {
Revision range1 = getRevisionSeen(o1);
Revision range2 = getRevisionSeen(o2);
if (range1 == FUTURE && range2 == FUTURE) {
- return o1.compareRevisionTime(o2);
+ return o1.compareRevisionTimeThenClusterId(o2);
}
if (range1 == null || range2 == null) {
- return o1.compareRevisionTime(o2);
+ return o1.compareRevisionTimeThenClusterId(o2);
}
- int comp = range1.compareRevisionTime(range2);
+ int comp = range1.compareRevisionTimeThenClusterId(range2);
if (comp != 0) {
return comp;
}
return Integer.signum(o1.getClusterId() - o2.getClusterId());
}
-
+
/**
- * Get the timestamp from the revision range, if found. If no range was
- * found for this cluster instance, or if the revision is older than the
- * earliest range, then 0 is returned. If the revision is newer than the
- * newest range for this cluster instance, then Long.MAX_VALUE is
- * returned.
- *
+ * Get the seen-at revision from the revision range.
+ * <p>
+ * <ul>
+ * <li>
+ * {@code null} if the revision is older than the earliest range
+ * </li>
+ * <li>
+ * if the revision is newer than the lower bound of the newest
+ * range, then {@link #NEWEST} is returned for a local cluster
+ * revision and {@link #FUTURE} for a foreign cluster revision.
+ * </li>
+ * <li>
+ * if the revision matches the lower seen-at bound of a range,
+ * then this seen-at revision is returned.
+ * </li>
+ * <li>
+ * otherwise the lower bound seen-at revision of next higher
+ * range is returned.
+ * </li>
+ * </ul>
+ *
* @param r the revision
- * @return the revision where it was seen, null if not found,
- * the timestamp plus 1 second for new local revisions;
- * Long.MAX_VALUE for new non-local revisions (meaning 'in the future')
+ * @return the seen-at revision or {@code null} if the revision is older
+ * than the earliest range.
*/
- private Revision getRevisionSeen(Revision r) {
+ Revision getRevisionSeen(Revision r) {
List<RevisionRange> list = map.get(r.getClusterId());
if (list == null) {
if (r.getClusterId() != currentClusterNodeId) {
@@ -464,25 +516,27 @@ public class Revision {
// search from latest backward
// (binary search could be used, but we expect most queries
// at the end of the list)
- Revision result = null;
for (int i = list.size() - 1; i >= 0; i--) {
RevisionRange range = list.get(i);
int compare = r.compareRevisionTime(range.revision);
- if (compare > 0) {
+ if (compare == 0) {
+ return range.seenAt;
+ } else if (compare > 0) {
if (i == list.size() - 1) {
// newer than the newest range
if (r.getClusterId() == currentClusterNodeId) {
// newer than all others, except for FUTURE
return NEWEST;
}
- // happenes in the future (not visible yet)
+ // happens in the future (not visible yet)
return FUTURE;
+ } else {
+ // there is a newer range
+ return list.get(i + 1).seenAt;
}
- break;
}
- result = range.seenAt;
}
- return result;
+ return null;
}
@Override
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/StableRevisionComparator.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/StableRevisionComparator.java?rev=1550438&r1=1550437&r2=1550438&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/StableRevisionComparator.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/StableRevisionComparator.java Thu Dec 12 15:39:37 2013
@@ -34,10 +34,6 @@ public class StableRevisionComparator im
@Override
public int compare(Revision o1, Revision o2) {
- int comp = o1.compareRevisionTime(o2);
- if (comp != 0) {
- return comp;
- }
- return Integer.signum(o1.getClusterId() - o2.getClusterId());
+ return o1.compareRevisionTimeThenClusterId(o2);
}
}
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java?rev=1550438&r1=1550437&r2=1550438&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java Thu Dec 12 15:39:37 2013
@@ -200,7 +200,7 @@ public class DocumentSplitTest extends B
Revision previous = null;
for (Map.Entry<Revision, String> entry : revs.entrySet()) {
if (previous != null) {
- assertTrue(previous.compareRevisionTime(entry.getKey()) > 0);
+ assertTrue(previous.compareRevisionTimeThenClusterId(entry.getKey()) > 0);
}
previous = entry.getKey();
}
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RandomizedClusterTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RandomizedClusterTest.java?rev=1550438&r1=1550437&r2=1550438&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RandomizedClusterTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RandomizedClusterTest.java Thu Dec 12 15:39:37 2013
@@ -344,8 +344,6 @@ public class RandomizedClusterTest {
if (conflictExpected) {
ok = false;
ex = "conflict expected";
- // afterwards, this cluster node should synchronize
- unseenChanges[mkId].clear();
} else {
ok = true;
}
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java?rev=1550438&r1=1550437&r2=1550438&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java Thu Dec 12 15:39:37 2013
@@ -18,11 +18,11 @@ package org.apache.jackrabbit.oak.plugin
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import org.apache.jackrabbit.oak.plugins.mongomk.Revision.RevisionComparator;
-import org.junit.Ignore;
import org.junit.Test;
/**
@@ -122,6 +122,9 @@ public class RevisionTest {
RevisionComparator comp = new RevisionComparator(0);
+ Revision r0c1 = new Revision(0x010, 0, 1);
+ Revision r0c2 = new Revision(0x010, 0, 2);
+
Revision r1c1 = new Revision(0x110, 0, 1);
Revision r2c1 = new Revision(0x120, 0, 1);
Revision r3c1 = new Revision(0x130, 0, 1);
@@ -142,6 +145,8 @@ public class RevisionTest {
"1:\n r120-0-1:r20-0-0\n" +
"2:\n r200-0-2:r10-0-0\n", comp.toString());
+ assertEquals(-1, comp.compare(r0c1, r0c2));
+
assertEquals(1, comp.compare(r1c1, r1c2));
assertEquals(1, comp.compare(r2c1, r2c2));
// both r3cx are still "in the future"
@@ -186,10 +191,6 @@ public class RevisionTest {
}
- /**
- * OAK-1274
- */
- @Ignore
@Test
public void clusterCompare() {
RevisionComparator comp = new RevisionComparator(1);
@@ -198,12 +199,54 @@ public class RevisionTest {
Revision r1c1 = new Revision(0x10, 0, 1);
Revision r1c2 = new Revision(0x20, 0, 2);
Revision r2c1 = new Revision(0x30, 0, 1);
-
+ Revision r2c2 = new Revision(0x40, 0, 2);
+
comp.add(r1c1, new Revision(0x10, 0, 0));
comp.add(r2c1, new Revision(0x20, 0, 0));
- // there's no range for r1c2 and must
- // be considered in the past
+ // there's no range for c2, and therefore this
+ // revision must be considered to be in the future
+ assertTrue(comp.compare(r1c2, r2c1) > 0);
+
+ // add a range for r2r2
+ comp.add(r2c2, new Revision(0x30, 0, 0));
+
+ // now there is a range for c2, but the revision is old,
+ // so it must be considered to be in the past
assertTrue(comp.compare(r1c2, r2c1) < 0);
}
+
+ @Test
+ public void revisionSeen() {
+ RevisionComparator comp = new RevisionComparator(1);
+
+ Revision r0 = new Revision(0x01, 0, 1);
+ Revision r1 = new Revision(0x10, 0, 1);
+ Revision r2 = new Revision(0x20, 0, 1);
+ Revision r21 = new Revision(0x21, 0, 1);
+ Revision r3 = new Revision(0x30, 0, 1);
+ Revision r4 = new Revision(0x40, 0, 1);
+ Revision r5 = new Revision(0x50, 0, 1);
+
+ comp.add(r1, new Revision(0x10, 0, 0));
+ comp.add(r2, new Revision(0x20, 0, 0));
+ comp.add(r3, new Revision(0x30, 0, 0));
+ comp.add(r4, new Revision(0x40, 0, 0));
+
+ // older than first range -> must return null
+ assertNull(comp.getRevisionSeen(r0));
+
+ // exact range start matches
+ assertEquals(new Revision(0x10, 0, 0), comp.getRevisionSeen(r1));
+ assertEquals(new Revision(0x20, 0, 0), comp.getRevisionSeen(r2));
+ assertEquals(new Revision(0x30, 0, 0), comp.getRevisionSeen(r3));
+ assertEquals(new Revision(0x40, 0, 0), comp.getRevisionSeen(r4));
+
+ // revision newer than most recent range -> NEWEST
+ assertEquals(RevisionComparator.NEWEST, comp.getRevisionSeen(r5));
+
+ // within a range -> must return lower bound of next higher range
+ assertEquals(new Revision(0x30, 0, 0), comp.getRevisionSeen(r21));
+ }
+
}