mreutegg commented on a change in pull request #359:
URL: https://github.com/apache/jackrabbit-oak/pull/359#discussion_r703558877
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfoTest.java
##########
@@ -417,6 +526,29 @@ public void skipExpiredClusterIdWithDifferentInstanceId()
throws Exception {
info2.dispose();
}
+ // OAK-9564
+ @Test
+ public void cannotGetClusterWithRuntimeId() {
+ ClusterNodeInfo info = newClusterNodeInfo(0);
+ int id = info.getId();
+ assertEquals(1, id);
+ // shut it down
+ info.dispose();
+
+ // edit the runtime ID
+ UpdateOp op = new UpdateOp(String.valueOf(id), false);
+ op.set(ClusterNodeInfo.RUNTIME_ID_KEY, "some-different-uuid");
+ assertNotNull(store.findAndUpdate(Collection.CLUSTER_NODES, op));
+
+ // shouldn't be able to acquire it again
+ try {
+ info = newClusterNodeInfo(0);
Review comment:
Should rather try to get the specific entry with the id.
```suggestion
info = newClusterNodeInfo(id);
```
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfoTest.java
##########
@@ -268,6 +270,113 @@ public void renewLeaseTimedOutWithCheck() throws
Exception {
}
}
+ // OAK-9564
+ @Test
+ public void renewLeaseSameRuntimeId() throws Exception {
+ ClusterNodeInfo info = newClusterNodeInfo(1);
+ String runtimeId = info.getRuntimeId();
+ long leaseEnd = info.getLeaseEndTime();
+ waitLeaseUpdateInterval();
+ assertTrue(info.renewLease());
+ assertTrue(info.getLeaseEndTime() > leaseEnd);
+ // The Runtime UUID should remain the same
+ assertEquals(info.getRuntimeId(), runtimeId);
+ assertFalse(handler.isLeaseFailure());
+ }
+
+ // OAK-9564
+ @Test
+ public void renewLeaseDifferentRuntimeId() throws Exception {
+ ClusterNodeInfo info = newClusterNodeInfo(1);
+ waitLeaseUpdateInterval();
+ long leaseEndTimeBeforeRenew = info.getLeaseEndTime();
+
+ // Modify the UUID to mock it belongs to a different node
+ UpdateOp update = new UpdateOp("1", false);
+ update.set(ClusterNodeInfo.RUNTIME_ID_KEY, "different-uuid");
+ store.findAndUpdate(Collection.CLUSTER_NODES, update);
+
+ try {
+ info.renewLease();
+ fail("Could not update lease anymore");
+ } catch(DocumentStoreException e) {
+ // expected
+ }
+
+ // Lease end time shouldn't be different
+ assertEquals(leaseEndTimeBeforeRenew, info.getLeaseEndTime());
+ }
+
+ // OAK-9564
+ @Test
+ public void renewLeaseTakingLongerThanTimeout() throws Exception {
+ ClusterNodeInfo info = newClusterNodeInfo(1);
+ waitLeaseUpdateInterval();
+ final long leaseEndTimeBeforeRenew = info.getLeaseEndTime();
+ final String runtimeId = info.getRuntimeId();
+
+ Map<String, Long> unexpectedLeaseEnd = new HashMap<>();
+ unexpectedLeaseEnd.put(ClusterNodeInfo.LEASE_END_KEY,
info.getLeaseEndTime() + 133333);
+
+ // The update will fail after 30 seconds. Simulating a Mongo timeout.
+ store.setFailAfterUpdate(1);
+ store.setDelayMillisOnce(30000);
+ store.setDelayMillis(10000);
+ store.setFindShouldAlterReturnDocument(true);
+ // However, the following find after the update will return an
+ // unexpected lease time (but still within a valid time).
+ // This unexpected update could come from a previous but very slow
update
+ // executed in Mongo. So it's still a valid one, but not the new one
+ // that is expected.
+ store.setMapAlterReturnDocument(unexpectedLeaseEnd);
+
+ // However, the current behaviour is that as the lease end time doesn't
+ // match the expected one, the lease will fail and the nodeStore
becomes
+ // unusable.
+ try {
+ info.renewLease();
+ } catch(DocumentStoreException e) {
+ // expected
+ }
+
+ // The new leaseEndTime coming from Mongo is not reflected in the
+ // ClusterNodeInfo. Meaning it will eventually be treated as 'expired'
+ // by the DocumentNodeStore, even when in Mongo it was set.
+ assertThat(leaseEndTimeBeforeRenew, lessThan(info.getLeaseEndTime()));
+ // Runtime ID is the same
+ assertEquals(runtimeId, info.getRuntimeId());
+ }
+
+ // OAK-9564: This is a someway artificial test. The idea behind is to try
to reproduce
Review comment:
Is this the only reason why this PR also introduces a new
`UpdateOp.lessThan()` condition on the `leaseEnd` time when it updates the
lease? Instead of introducing a new condition, do you think it's possible to
use `UpdateOp.max()`? This would also ensure `leaseEnd` does not go back in
time.
##########
File path:
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfoDocument.java
##########
@@ -69,6 +69,13 @@ public long getStartTime() {
return startTime;
}
+ /**
+ * @return the Runtime ID for this cluster node.
+ */
+ public String getRuntimeId() {
Review comment:
Please annotate with Nullable.
##########
File path:
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfo.java
##########
@@ -1066,9 +1084,9 @@ public boolean renewLease() throws DocumentStoreException
{
"anymore. {}", id, doc);
// break here and let the next lease update attempt fail
break;
- } else if (doc.getLeaseEndTime() == previousLeaseEndTime
- || doc.getLeaseEndTime() == updatedLeaseEndTime) {
- // set lease end times to current values
+ } else if (doc.getRuntimeId().equals(runtimeId)) {
+ // set lease end times to current values, as they belong
+ // to this same cluster node
previousLeaseEndTime = doc.getLeaseEndTime();
Review comment:
`previousLeaseEndTime` does not need to be maintained anymore.
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfoTest.java
##########
@@ -268,6 +270,113 @@ public void renewLeaseTimedOutWithCheck() throws
Exception {
}
}
+ // OAK-9564
+ @Test
+ public void renewLeaseSameRuntimeId() throws Exception {
+ ClusterNodeInfo info = newClusterNodeInfo(1);
+ String runtimeId = info.getRuntimeId();
+ long leaseEnd = info.getLeaseEndTime();
+ waitLeaseUpdateInterval();
+ assertTrue(info.renewLease());
+ assertTrue(info.getLeaseEndTime() > leaseEnd);
+ // The Runtime UUID should remain the same
+ assertEquals(info.getRuntimeId(), runtimeId);
+ assertFalse(handler.isLeaseFailure());
+ }
+
+ // OAK-9564
+ @Test
+ public void renewLeaseDifferentRuntimeId() throws Exception {
+ ClusterNodeInfo info = newClusterNodeInfo(1);
+ waitLeaseUpdateInterval();
+ long leaseEndTimeBeforeRenew = info.getLeaseEndTime();
+
+ // Modify the UUID to mock it belongs to a different node
+ UpdateOp update = new UpdateOp("1", false);
+ update.set(ClusterNodeInfo.RUNTIME_ID_KEY, "different-uuid");
+ store.findAndUpdate(Collection.CLUSTER_NODES, update);
+
+ try {
+ info.renewLease();
+ fail("Could not update lease anymore");
+ } catch(DocumentStoreException e) {
+ // expected
+ }
+
+ // Lease end time shouldn't be different
+ assertEquals(leaseEndTimeBeforeRenew, info.getLeaseEndTime());
+ }
+
+ // OAK-9564
+ @Test
+ public void renewLeaseTakingLongerThanTimeout() throws Exception {
+ ClusterNodeInfo info = newClusterNodeInfo(1);
+ waitLeaseUpdateInterval();
+ final long leaseEndTimeBeforeRenew = info.getLeaseEndTime();
+ final String runtimeId = info.getRuntimeId();
+
+ Map<String, Long> unexpectedLeaseEnd = new HashMap<>();
+ unexpectedLeaseEnd.put(ClusterNodeInfo.LEASE_END_KEY,
info.getLeaseEndTime() + 133333);
+
+ // The update will fail after 30 seconds. Simulating a Mongo timeout.
+ store.setFailAfterUpdate(1);
+ store.setDelayMillisOnce(30000);
+ store.setDelayMillis(10000);
+ store.setFindShouldAlterReturnDocument(true);
+ // However, the following find after the update will return an
+ // unexpected lease time (but still within a valid time).
+ // This unexpected update could come from a previous but very slow
update
+ // executed in Mongo. So it's still a valid one, but not the new one
+ // that is expected.
+ store.setMapAlterReturnDocument(unexpectedLeaseEnd);
Review comment:
These do not seem to have an effect. Values are set, but never used.
##########
File path:
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateOp.java
##########
@@ -557,6 +590,16 @@ public static Condition newNotEqualsCondition(@Nullable
Object value) {
return new Condition(Type.NOTEQUALS, value);
}
+ /**
+ * Creates a new lessThan condition with the given value.
+ *
+ * @param value the value to compare t
+ * @return the lessThan condition.
+ */
+ public static Condition newLessThanCondition(@Nullable Object value) {
Review comment:
What is the semantic when `value` is `null`? I think it would be better
if `value` is of type `Comparable<T>`, similar to `UpdateOp.max()`.
I'd also like to better understand if this addition is really necessary.
Adding this functionality should also have more test coverage. E.g. additional
tests in `BasicDocumentStoreTest`.
##########
File path:
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/UpdateUtils.java
##########
@@ -150,6 +150,18 @@ public static boolean checkConditions(@NotNull Document
doc,
} else if (c.type == Condition.Type.NOTEQUALS && equal) {
return false;
}
+ } else if (c.type == Condition.Type.LESSTHAN) {
+ if (r != null) {
+ if (value instanceof Map) {
+ value = ((Map) value).get(r);
+ } else {
+ value = null;
+ }
+ }
+ if (value instanceof Comparable && c.value instanceof
Comparable) {
+ int lessThan = ((Comparable) value).compareTo(c.value);
+ return lessThan == -1;
+ }
Review comment:
I think this should not silently ignore cases when value is not
Comparable, but this could be enforced earlier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
