Index: src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java (revision 1550368) +++ src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java (working copy) @@ -72,10 +72,19 @@ /** * Compare the time part of two revisions. If they contain the same time, * the counter is compared. + *

+ * 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; @@ -84,6 +93,35 @@ } /** + * 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 @@ -427,29 +465,38 @@ 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. + *

+ *

+ * * @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) { List list = map.get(r.getClusterId()); @@ -464,25 +511,25 @@ // 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; } - break; + return range.seenAt; } - result = range.seenAt; } - return result; + return null; } @Override Index: src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/StableRevisionComparator.java =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/StableRevisionComparator.java (revision 1550368) +++ src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/StableRevisionComparator.java (working copy) @@ -34,10 +34,6 @@ @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); } } Index: src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java (revision 1550368) +++ src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java (working copy) @@ -200,7 +200,7 @@ Revision previous = null; for (Map.Entry entry : revs.entrySet()) { if (previous != null) { - assertTrue(previous.compareRevisionTime(entry.getKey()) > 0); + assertTrue(previous.compareRevisionTimeThenClusterId(entry.getKey()) > 0); } previous = entry.getKey(); } Index: src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java (revision 1550368) +++ src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java (working copy) @@ -122,6 +122,9 @@ 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 @@ "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 @@ } - /** - * OAK-1274 - */ - @Ignore @Test public void clusterCompare() { RevisionComparator comp = new RevisionComparator(1); @@ -198,12 +199,21 @@ 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); } + }