[
https://issues.apache.org/jira/browse/OAK-1274?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13845462#comment-13845462
]
Thomas Mueller commented on OAK-1274:
-------------------------------------
I have a patch, but one of the tests fails now for me, and I am not sure why
(the org.apache.jackrabbit.oak.plugins.mongomk.RandomizedClusterTest):
{noformat}
Index: src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java
===================================================================
--- src/main/java/org/apache/jackrabbit/oak/plugins/mongomk/Revision.java
(revision 1550150)
+++ 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.
+ * <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;
@@ -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,12 +465,12 @@
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;
}
@@ -464,11 +502,12 @@
// 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) {
@@ -478,11 +517,10 @@
// happenes 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 1550150)
+++
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 1550150)
+++
src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/DocumentSplitTest.java
(working copy)
@@ -200,7 +200,7 @@
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();
}
Index: src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java
===================================================================
--- src/test/java/org/apache/jackrabbit/oak/plugins/mongomk/RevisionTest.java
(revision 1550150)
+++ 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);
}
+
}
{noformat}
> Wrong comparison for old Revisions
> ----------------------------------
>
> Key: OAK-1274
> URL: https://issues.apache.org/jira/browse/OAK-1274
> Project: Jackrabbit Oak
> Issue Type: Bug
> Components: core, mongomk
> Affects Versions: 0.13
> Reporter: Marcel Reutegger
> Priority: Minor
>
> Comparing revisions may return wrong results when one of the revision is
> older than the oldest revision range kept in the RevisionComparator.
> There are multiple reasons why there is no revision range for a revision.
> - The revision was created more than an hour ago and the associated range was
> purged from the RevisionComparator. This happens in the background thread of
> MongoNodeStore with a threshold of one hour.
> - The revision isn't actually that old, but still included in a range of the
> RevisionComparator because the MongoNodeStore instance was just initialized
> and only has ranges for recent revisions but not for those before the init.
--
This message was sent by Atlassian JIRA
(v6.1.4#6159)
