From 1c3cf5311f70034caf2c3348c26c989117061ad7 Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Sat, 30 Aug 2025 03:00:01 +0000 Subject: [PATCH 1/2] Added test cases for ServiceLock methods Added tests for determining if the lock ownership method is working or getting stuck in a loop when the createdNodeName is null. --- .../core/fate/zookeeper/ServiceLockTest.java | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java b/core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java index 44b6fcd4cec..fe423b06bcb 100644 --- a/core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java +++ b/core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java @@ -18,18 +18,39 @@ */ package org.apache.accumulo.core.fate.zookeeper; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.easymock.EasyMock.anyObject; +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.eq; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.verify; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; import java.util.UUID; +import org.apache.accumulo.core.fate.zookeeper.ServiceLock.AccumuloLockWatcher; +import org.apache.accumulo.core.fate.zookeeper.ServiceLock.LockWatcher; +import org.apache.accumulo.core.fate.zookeeper.ServiceLock.ServiceLockPath; +import org.apache.accumulo.core.util.ServerServices; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.Stat; +import org.easymock.EasyMock; import org.junit.jupiter.api.Test; public class ServiceLockTest { + private final String ZPATH = "/test/"; + private final ServiceLockPath path = ServiceLock.path(ZPATH); + @Test public void testSortAndFindLowestPrevPrefix() { List children = new ArrayList<>(); @@ -103,4 +124,73 @@ public void uuidTest() { assertTrue(candidate.contains(uuid)); assertTrue(candidate.contains(seq)); } + + @Test + public void determineLockOwnershipTest() throws Exception { + final long ephemeralOwner = 123456789L; + Stat existsStat = new Stat(); + existsStat.setEphemeralOwner(ephemeralOwner); + + ZooKeeper zk = createMock(ZooKeeper.class); + expect(zk.exists(eq(ZPATH), anyObject(ServiceLock.class))).andReturn(existsStat); + + replay(zk); + + ServiceLock serviceLock = new ServiceLock(zk, path, UUID.randomUUID()); + AccumuloLockWatcher mockLockWatcher = EasyMock.createMock(AccumuloLockWatcher.class); + + Method determineLockOwnershipMethod = ServiceLock.class + .getDeclaredMethod("determineLockOwnership", String.class, AccumuloLockWatcher.class); + determineLockOwnershipMethod.setAccessible(true); + + String testEphemeralNode = "zlock#test-uuid#0000000001"; + + InvocationTargetException exception = assertThrows(InvocationTargetException.class, () -> { + determineLockOwnershipMethod.invoke(serviceLock, testEphemeralNode, mockLockWatcher); + }); + + assertEquals(exception.getTargetException().getClass(), IllegalStateException.class); + assertTrue(exception.getTargetException().getMessage() + .contains("Called determineLockOwnership() when ephemeralNodeName == null")); + + verify(zk); + } + + @Test + public void tryLockCleanupOnFailureTest() throws Exception { + LockWatcher mockLockWatcher = EasyMock.createMock(LockWatcher.class); + + final long ephemeralOwner = 123456789L; + Stat existsStat = new Stat(); + existsStat.setEphemeralOwner(ephemeralOwner); + + ZooKeeper zk = createMock(ZooKeeper.class); + expect(zk.exists(eq(ZPATH), anyObject(ServiceLock.class))).andReturn(existsStat); + + replay(zk); + + ServiceLock serviceLock = new ServiceLock(zk, path, UUID.randomUUID()); + + Field createdNodeNameField = ServiceLock.class.getDeclaredField("createdNodeName"); + createdNodeNameField.setAccessible(true); + createdNodeNameField.set(serviceLock, "zlock#test-uuid#0000000001"); + + Field lockWasAcquiredField = ServiceLock.class.getDeclaredField("lockWasAcquired"); + lockWasAcquiredField.setAccessible(true); + lockWasAcquiredField.set(serviceLock, false); + + assertThrows(IllegalStateException.class, + () -> serviceLock.tryLock(mockLockWatcher, + new ServerServices("9443", ServerServices.Service.COMPACTOR_CLIENT).toString() + .getBytes(UTF_8))); + + createdNodeNameField.setAccessible(true); + // Cast to string since field should be set to null + String nullCreatedNodeName = (String) createdNodeNameField.get(serviceLock); + // This should be null, but instead is not null. + // assertNull(nullCreatedNodeName, "createdNodeName was not cleaned up after failed lock + // attempt"); + assertNotNull(nullCreatedNodeName, + "createdNodeName was not cleaned up after failed lock attempt"); + } } From 5213fa2a7c47014ae772bfddcd017ff62bfafbd0 Mon Sep 17 00:00:00 2001 From: Daniel Roberts ddanielr Date: Tue, 2 Sep 2025 16:21:29 +0000 Subject: [PATCH 2/2] Handles auto cleanup after illegalStateException Adds a catch statement to the lock method so the createdNodeName entry is cleaned up after the tryLock hits an illegalStateException and cannot get out of it. --- .../core/fate/zookeeper/ServiceLock.java | 6 ++++- .../core/fate/zookeeper/ServiceLockTest.java | 25 ++++++++++--------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java b/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java index c6478baff36..164b52f514a 100644 --- a/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java +++ b/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java @@ -155,7 +155,11 @@ public synchronized boolean tryLock(LockWatcher lw, byte[] data) LockWatcherWrapper lww = new LockWatcherWrapper(lw); - lock(lww, data); + try { + lock(lww, data); + } catch (IllegalStateException e) { + lww.failedToAcquireLock(e); + } if (lww.acquiredLock) { return true; diff --git a/core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java b/core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java index fe423b06bcb..18474230723 100644 --- a/core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java +++ b/core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java @@ -23,10 +23,11 @@ import static org.easymock.EasyMock.createMock; import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; import static org.easymock.EasyMock.replay; import static org.easymock.EasyMock.verify; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -48,7 +49,7 @@ public class ServiceLockTest { - private final String ZPATH = "/test/"; + private final String ZPATH = "/test"; private final ServiceLockPath path = ServiceLock.path(ZPATH); @Test @@ -161,11 +162,15 @@ public void tryLockCleanupOnFailureTest() throws Exception { LockWatcher mockLockWatcher = EasyMock.createMock(LockWatcher.class); final long ephemeralOwner = 123456789L; + final String zlock = "zlock#test-uuid#0000000001"; Stat existsStat = new Stat(); existsStat.setEphemeralOwner(ephemeralOwner); ZooKeeper zk = createMock(ZooKeeper.class); expect(zk.exists(eq(ZPATH), anyObject(ServiceLock.class))).andReturn(existsStat); + expect(zk.getChildren(eq(ZPATH + "/" + zlock), anyObject(null))).andReturn(List.of()); + zk.delete(eq(ZPATH + "/" + zlock), eq(-1)); + expectLastCall(); replay(zk); @@ -173,24 +178,20 @@ public void tryLockCleanupOnFailureTest() throws Exception { Field createdNodeNameField = ServiceLock.class.getDeclaredField("createdNodeName"); createdNodeNameField.setAccessible(true); - createdNodeNameField.set(serviceLock, "zlock#test-uuid#0000000001"); + createdNodeNameField.set(serviceLock, zlock); Field lockWasAcquiredField = ServiceLock.class.getDeclaredField("lockWasAcquired"); lockWasAcquiredField.setAccessible(true); lockWasAcquiredField.set(serviceLock, false); - assertThrows(IllegalStateException.class, - () -> serviceLock.tryLock(mockLockWatcher, - new ServerServices("9443", ServerServices.Service.COMPACTOR_CLIENT).toString() - .getBytes(UTF_8))); + serviceLock.tryLock(mockLockWatcher, + new ServerServices("9443", ServerServices.Service.COMPACTOR_CLIENT).toString() + .getBytes(UTF_8)); createdNodeNameField.setAccessible(true); // Cast to string since field should be set to null String nullCreatedNodeName = (String) createdNodeNameField.get(serviceLock); - // This should be null, but instead is not null. - // assertNull(nullCreatedNodeName, "createdNodeName was not cleaned up after failed lock - // attempt"); - assertNotNull(nullCreatedNodeName, - "createdNodeName was not cleaned up after failed lock attempt"); + assertNull(nullCreatedNodeName, "createdNodeName was not cleaned up after failed lock"); + verify(zk); } }