mreutegg commented on a change in pull request #289: URL: https://github.com/apache/jackrabbit-oak/pull/289#discussion_r627567459
########## File path: oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/RecoveryLockTest.java ########## @@ -182,6 +185,49 @@ public void selfRecoveryWithinDeadline() throws Exception { assertFalse(lock1.acquireRecoveryLock(2)); } + @Test + // OAK-9401: Reproduce a bug that happens when the cluster is not active having a null leaseEndTime + public void breakRecoveryLockOfNotActiveCluster() throws Exception { + // Create a mocked version of the DocumentStore, to replace a method later during the test + DocumentStore store = spy(new MemoryDocumentStore()); + + info1 = ClusterNodeInfo.getInstance(store, RecoveryHandler.NOOP, + null, "node1", 1); + RecoveryLock recLock = new RecoveryLock(store, clock, 1); + + // expire clusterId 1 + clock.waitUntil(info1.getLeaseEndTime() + DEFAULT_LEASE_UPDATE_INTERVAL_MILLIS); + MissingLastRevSeeker seeker = new MissingLastRevSeeker(store, clock); + + Semaphore recovering = new Semaphore(0); + Semaphore recovered = new Semaphore(0); + // simulate new startup and get info again + Future<ClusterNodeInfo> infoFuture = executor.submit(() -> + ClusterNodeInfo.getInstance(store, clusterId -> { + assertTrue(recLock.acquireRecoveryLock(1)); + recovering.release(); + recovered.acquireUninterruptibly(); + recLock.releaseRecoveryLock(true); + return true; + }, null, "node1", 1)); + // wait until submitted task is in recovery + recovering.acquireUninterruptibly(); + + // OAK-9401: Reproduce a bug that happens when the cluster is not active having a null leaseEndTime + // create a mocked copy of the ClusterNodeInfoDocument to be able to edit it, as the original is immutable + ClusterNodeInfoDocument realClusterInfo = spy(store.find(CLUSTER_NODES, String.valueOf(1))); + ClusterNodeInfoDocument mockClusterInfo = spy(CLUSTER_NODES.newDocument(store)); + realClusterInfo.deepCopy(mockClusterInfo); + + mockClusterInfo.put(ClusterNodeInfo.LEASE_END_KEY, null); + doReturn(false).when(mockClusterInfo).isActive(); + when(store.find(CLUSTER_NODES, String.valueOf(1))).thenCallRealMethod().thenReturn(mockClusterInfo); + + // clusterId 2 should be able to acquire (break) the recovery lock, instead of + // throwing "java.lang.NullPointerException: Lease End Time not set" + assertTrue(recLock.acquireRecoveryLock(2)); + } Review comment: IIUC, the submitted task is still blocked when the test finishes. ```suggestion // let submitted task complete recovered.release(); } ``` ########## File path: oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/RecoveryLockTest.java ########## @@ -182,6 +185,49 @@ public void selfRecoveryWithinDeadline() throws Exception { assertFalse(lock1.acquireRecoveryLock(2)); } + @Test + // OAK-9401: Reproduce a bug that happens when the cluster is not active having a null leaseEndTime + public void breakRecoveryLockOfNotActiveCluster() throws Exception { + // Create a mocked version of the DocumentStore, to replace a method later during the test + DocumentStore store = spy(new MemoryDocumentStore()); + + info1 = ClusterNodeInfo.getInstance(store, RecoveryHandler.NOOP, + null, "node1", 1); + RecoveryLock recLock = new RecoveryLock(store, clock, 1); + + // expire clusterId 1 + clock.waitUntil(info1.getLeaseEndTime() + DEFAULT_LEASE_UPDATE_INTERVAL_MILLIS); + MissingLastRevSeeker seeker = new MissingLastRevSeeker(store, clock); Review comment: This is never used. Remove? ```suggestion ``` ########## File path: oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/RecoveryLockTest.java ########## @@ -182,6 +185,49 @@ public void selfRecoveryWithinDeadline() throws Exception { assertFalse(lock1.acquireRecoveryLock(2)); } + @Test + // OAK-9401: Reproduce a bug that happens when the cluster is not active having a null leaseEndTime + public void breakRecoveryLockOfNotActiveCluster() throws Exception { + // Create a mocked version of the DocumentStore, to replace a method later during the test + DocumentStore store = spy(new MemoryDocumentStore()); + + info1 = ClusterNodeInfo.getInstance(store, RecoveryHandler.NOOP, + null, "node1", 1); + RecoveryLock recLock = new RecoveryLock(store, clock, 1); + + // expire clusterId 1 + clock.waitUntil(info1.getLeaseEndTime() + DEFAULT_LEASE_UPDATE_INTERVAL_MILLIS); + MissingLastRevSeeker seeker = new MissingLastRevSeeker(store, clock); + + Semaphore recovering = new Semaphore(0); + Semaphore recovered = new Semaphore(0); + // simulate new startup and get info again + Future<ClusterNodeInfo> infoFuture = executor.submit(() -> Review comment: Similar here. `infoFuture` is not used later on. ```suggestion executor.submit(() -> ``` -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org