mreutegg commented on a change in pull request #359:
URL: https://github.com/apache/jackrabbit-oak/pull/359#discussion_r719984259
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfoTest.java
##########
@@ -269,6 +271,110 @@ 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<>();
+ long unexpectedLeaseEndTime = info.getLeaseEndTime() + 133333;
+ unexpectedLeaseEnd.put(ClusterNodeInfo.LEASE_END_KEY,
unexpectedLeaseEndTime);
+
+ // 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()));
+ assertEquals(unexpectedLeaseEndTime, 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
+ // a case where a renewLease fails because of a timeout. Then the
following renewLease
+ // occurs faster, but during that time the previous update is executed in
Mongo.
+ // That 'older' update shouldn't go through now, reducing the effective
lease end time.
+ @Test
+ public void renewLeaseShouldNotGoBackInTime() throws Exception {
+ ClusterNodeInfo info = newClusterNodeInfo(1);
+ long leaseEnd = info.getLeaseEndTime();
+ waitLeaseUpdateInterval();
+
+ long newerLeaseEndTime = clock.getTime() +
ClusterNodeInfo.DEFAULT_LEASE_DURATION_MILLIS +
+ ClusterNodeInfo.DEFAULT_LEASE_UPDATE_INTERVAL_MILLIS;
+ // simulate a newer renew lease took place
+ UpdateOp update = new UpdateOp("1", false);
+ update.set(ClusterNodeInfo.LEASE_END_KEY, newerLeaseEndTime);
+ store.findAndUpdate(Collection.CLUSTER_NODES, update);
+
+ // now another renew happens, which will try to set a lesser lease end
+ info.renewLease();
+
+ Document info2 = store.find(Collection.CLUSTER_NODES, "1");
+ // the lease end time should remain the same
+ assertEquals(newerLeaseEndTime,
info2.get(ClusterNodeInfo.LEASE_END_KEY));
Review comment:
Instead you can also do:
```suggestion
ClusterNodeInfoDocument info2 = store.find(Collection.CLUSTER_NODES,
"1");
assertNotNull(info2);
// the lease end time should remain the same
assertEquals(newerLeaseEndTime, info2.getLeaseEndTime());
```
##########
File path:
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfo.java
##########
@@ -1067,10 +1067,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
- previousLeaseEndTime = doc.getLeaseEndTime();
+ } else if (doc.getRuntimeId().equals(runtimeId)) {
Review comment:
This may produce a NullPointerException. `doc.getRuntimeId()` may return
`null`. Better flip it around.
```suggestion
} else if (runtimeId.equals(doc.getRuntimeId())) {
```
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfoTest.java
##########
@@ -269,6 +271,110 @@ 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");
Review comment:
```suggestion
fail("Should not update lease anymore");
```
##########
File path:
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfoTest.java
##########
@@ -269,6 +271,110 @@ 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<>();
+ long unexpectedLeaseEndTime = info.getLeaseEndTime() + 133333;
+ unexpectedLeaseEnd.put(ClusterNodeInfo.LEASE_END_KEY,
unexpectedLeaseEndTime);
+
+ // 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()));
+ assertEquals(unexpectedLeaseEndTime, 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
+ // a case where a renewLease fails because of a timeout. Then the
following renewLease
+ // occurs faster, but during that time the previous update is executed in
Mongo.
+ // That 'older' update shouldn't go through now, reducing the effective
lease end time.
+ @Test
+ public void renewLeaseShouldNotGoBackInTime() throws Exception {
+ ClusterNodeInfo info = newClusterNodeInfo(1);
+ long leaseEnd = info.getLeaseEndTime();
Review comment:
`leaseEnd` is unused -> remove
```suggestion
```
--
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]
