Fix error in uuid validation, checkstyle fixes (#3994)
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 3e8c444..63807d0 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
@@ -102,7 +102,7 @@
private LockWatcher lockWatcher;
private String lockNodeName;
private volatile boolean lockWasAcquired;
- private volatile boolean watchingParent = false;
+ private volatile boolean watchingParent;
private String createdNodeName;
private String watchingNodeName;
@@ -190,14 +190,18 @@
children.forEach(c -> {
LOG.trace("Validating {}", c);
if (c.startsWith(ZLOCK_PREFIX)) {
- String candidate = c.substring(ZLOCK_PREFIX.length() + 1);
+ String candidate = c.substring(ZLOCK_PREFIX.length());
if (candidate.contains("#")) {
int idx = candidate.indexOf('#');
- String uuid = candidate.substring(0, idx - 1);
+ String uuid = candidate.substring(0, idx);
String sequenceNum = candidate.substring(idx + 1);
try {
LOG.trace("Testing uuid format of {}", uuid);
- UUID.fromString(uuid);
+ // string check guards uuids like "1-1-1-1-1" that parse to
+ // "00000001-0001-0001-0001-000000000001"
+ if (!uuid.equals(UUID.fromString(uuid).toString())) {
+ throw new IllegalArgumentException(uuid + " is an invalid UUID");
+ }
if (sequenceNum.length() == 10) {
try {
LOG.trace("Testing number format of {}", sequenceNum);
@@ -277,7 +281,7 @@
List<String> children = validateAndSort(path, zooKeeper.getChildren(path.toString(), null));
- if (null == children || !children.contains(createdEphemeralNode)) {
+ if (!children.contains(createdEphemeralNode)) {
LOG.error("Expected ephemeral node {} to be in the list of children {}", createdEphemeralNode,
children);
throw new RuntimeException(
@@ -405,8 +409,7 @@
// were created but the client missed the response for some reason. Find the ephemeral nodes
// with this ZLOCK_UUID and lowest sequential number.
List<String> children = validateAndSort(path, zooKeeper.getChildren(path.toString(), null));
- if (null == children
- || !children.contains(createPath.substring(path.toString().length() + 1))) {
+ if (!children.contains(createPath.substring(path.toString().length() + 1))) {
LOG.error("Expected ephemeral node {} to be in the list of children {}", createPath,
children);
throw new RuntimeException("Lock attempt ephemeral node no longer exist " + createPath);
@@ -443,6 +446,9 @@
}
}
}
+ if (lowestSequentialPath == null) {
+ throw new IllegalStateException("Could not find lowest sequential path under " + path);
+ }
final String pathForWatcher = lowestSequentialPath;
// Set a watcher on the lowest sequential node that we created, this handles the case
@@ -633,7 +639,7 @@
var zLockPath = path(lid.path);
List<String> children = validateAndSort(zLockPath, zc.getChildren(zLockPath.toString()));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
return false;
}
@@ -651,7 +657,7 @@
List<String> children = validateAndSort(path, zk.getChildren(path.toString(), null));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
return null;
}
@@ -665,7 +671,7 @@
List<String> children = validateAndSort(path, zc.getChildren(path.toString()));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
return null;
}
@@ -682,7 +688,7 @@
List<String> children = validateAndSort(path, zc.getChildren(path.toString()));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
return 0;
}
@@ -714,7 +720,7 @@
List<String> children = validateAndSort(path, zk.getChildren(path.toString()));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
throw new IllegalStateException("No lock is held at " + path);
}
@@ -735,7 +741,7 @@
List<String> children = validateAndSort(path, zk.getChildren(path.toString()));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
throw new IllegalStateException("No lock is held at " + path);
}
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 d94c358..44b6fcd 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
@@ -20,16 +20,18 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.ArrayList;
import java.util.List;
+import java.util.UUID;
import org.junit.jupiter.api.Test;
public class ServiceLockTest {
@Test
- public void testSortAndFindLowestPrevPrefix() throws Exception {
+ public void testSortAndFindLowestPrevPrefix() {
List<String> children = new ArrayList<>();
children.add("zlock#00000000-0000-0000-0000-ffffffffffff#0000000007");
children.add("zlock#00000000-0000-0000-0000-eeeeeeeeeeee#0000000010");
@@ -66,15 +68,39 @@
ServiceLock.findLowestPrevPrefix(validChildren,
"zlock#00000000-0000-0000-0000-eeeeeeeeeeee#0000000010"));
- assertThrows(IndexOutOfBoundsException.class, () -> {
- ServiceLock.findLowestPrevPrefix(validChildren,
- "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001");
- });
+ assertThrows(IndexOutOfBoundsException.class,
+ () -> ServiceLock.findLowestPrevPrefix(validChildren,
+ "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001"));
- assertThrows(IndexOutOfBoundsException.class, () -> {
- ServiceLock.findLowestPrevPrefix(validChildren,
- "zlock#00000000-0000-0000-0000-XXXXXXXXXXXX#0000000099");
- });
+ assertThrows(IndexOutOfBoundsException.class,
+ () -> ServiceLock.findLowestPrevPrefix(validChildren,
+ "zlock#00000000-0000-0000-0000-XXXXXXXXXXXX#0000000099"));
}
+ @Test
+ public void rejectInvalidUUID() {
+ List<String> children = new ArrayList<>();
+ String uuid = "1-1-1-1-1";
+ String seq = "1234567891";
+ children.add("zlock#" + uuid + "#" + seq);
+
+ // pass as UUID, but fail on string compare.
+ assertEquals("00000001-0001-0001-0001-000000000001", UUID.fromString(uuid).toString());
+ final List<String> validChildren = ServiceLock.validateAndSort(ServiceLock.path(""), children);
+ assertEquals(0, validChildren.size());
+ }
+
+ @Test
+ public void uuidTest() {
+ List<String> children = new ArrayList<>();
+ String uuid = "219ad0f6-ebe0-416e-a20f-c0f32922841d";
+ String seq = "1234567891";
+ children.add("zlock#" + uuid + "#" + seq);
+
+ final List<String> validChildren = ServiceLock.validateAndSort(ServiceLock.path(""), children);
+ assertEquals(1, validChildren.size());
+ String candidate = validChildren.get(0);
+ assertTrue(candidate.contains(uuid));
+ assertTrue(candidate.contains(seq));
+ }
}
diff --git a/test/src/main/java/org/apache/accumulo/test/fate/zookeeper/ServiceLockIT.java b/test/src/main/java/org/apache/accumulo/test/fate/zookeeper/ServiceLockIT.java
index fd71842..7069ed5 100644
--- a/test/src/main/java/org/apache/accumulo/test/fate/zookeeper/ServiceLockIT.java
+++ b/test/src/main/java/org/apache/accumulo/test/fate/zookeeper/ServiceLockIT.java
@@ -44,6 +44,7 @@
import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
import org.apache.accumulo.core.fate.zookeeper.ZooSession;
import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.test.util.Wait;
import org.apache.accumulo.test.zookeeper.ZooKeeperTestingServer;
import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.KeeperException;
@@ -372,9 +373,7 @@
try (ZooKeeper zk = new ZooKeeper(szk.getConn(), 30000, watcher)) {
ZooUtil.digestAuth(zk, "secret");
- while (!watcher.isConnected()) {
- Thread.sleep(200);
- }
+ Wait.waitFor(() -> !watcher.isConnected(), 30_000, 200);
zk.create(parent.toString(), new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
@@ -422,13 +421,8 @@
ZooUtil.digestAuth(zk1, "secret");
ZooUtil.digestAuth(zk2, "secret");
- while (!watcher1.isConnected()) {
- Thread.sleep(200);
- }
-
- while (!watcher2.isConnected()) {
- Thread.sleep(200);
- }
+ Wait.waitFor(() -> !watcher1.isConnected(), 30_000, 200);
+ Wait.waitFor(() -> !watcher2.isConnected(), 30_000, 200);
// Create the parent node
zk1.createOnce(parent.toString(), new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE,
@@ -492,8 +486,6 @@
assertTrue(zlw2.isLockHeld());
zl2.unlock();
- zk2.close();
-
}
}
@@ -533,9 +525,8 @@
try (ZooKeeperWrapper zk = new ZooKeeperWrapper(szk.getConn(), 30000, watcher)) {
ZooUtil.digestAuth(zk, "secret");
- while (!watcher.isConnected()) {
- Thread.sleep(50);
- }
+ Wait.waitFor(() -> !watcher.isConnected(), 30_000, 50);
+
ServiceLock zl = getZooLock(zk, parent, uuid);
getLockLatch.countDown(); // signal we are done
getLockLatch.await(); // wait for others to finish
@@ -584,9 +575,8 @@
try (ZooKeeperWrapper zk = new ZooKeeperWrapper(szk.getConn(), 30000, watcher)) {
ZooUtil.digestAuth(zk, "secret");
- while (!watcher.isConnected()) {
- Thread.sleep(50);
- }
+ Wait.waitFor(() -> !watcher.isConnected(), 30_000, 50);
+
// Create the parent node
zk.createOnce(parent.toString(), new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
@@ -654,9 +644,7 @@
try (ZooKeeper zk = new ZooKeeper(szk.getConn(), 30000, watcher)) {
ZooUtil.digestAuth(zk, "secret");
- while (!watcher.isConnected()) {
- Thread.sleep(200);
- }
+ Wait.waitFor(() -> !watcher.isConnected(), 30_000, 200);
for (int i = 0; i < 10; i++) {
zk.create(parent.toString(), new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE,
@@ -692,9 +680,7 @@
try (ZooKeeper zk = new ZooKeeper(szk.getConn(), 30000, watcher)) {
ZooUtil.digestAuth(zk, "secret");
- while (!watcher.isConnected()) {
- Thread.sleep(200);
- }
+ Wait.waitFor(() -> !watcher.isConnected(), 30_000, 200);
zk.create(parent.toString(), new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
@@ -709,5 +695,4 @@
assertEquals("test2", new String(zk.getData(zl.getLockPath(), null, null)));
}
}
-
}
