This is an automated email from the ASF dual-hosted git repository.
edcoleman pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new 170202a3b9 Fix error in uuid validation, checkstyle fixes (#3994)
170202a3b9 is described below
commit 170202a3b9fbf1a654fdad25ac49d37cc53c519c
Author: EdColeman <d...@etcoleman.com>
AuthorDate: Wed Dec 6 12:51:49 2023 -0500
Fix error in uuid validation, checkstyle fixes (#3994)
---
.../accumulo/core/fate/zookeeper/ServiceLock.java | 32 +++++++++-------
.../core/fate/zookeeper/ServiceLockTest.java | 44 +++++++++++++++++-----
.../test/fate/zookeeper/ServiceLockIT.java | 35 +++++------------
3 files changed, 64 insertions(+), 47 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 3e8c444691..63807d0f37 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 @@ public class ServiceLock implements Watcher {
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 @@ public class ServiceLock implements Watcher {
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 @@ public class ServiceLock implements Watcher {
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 @@ public class ServiceLock implements Watcher {
// 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 @@ public class ServiceLock implements Watcher {
}
}
}
+ 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 @@ public class ServiceLock implements Watcher {
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 @@ public class ServiceLock implements Watcher {
List<String> children = validateAndSort(path,
zk.getChildren(path.toString(), null));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
return null;
}
@@ -665,7 +671,7 @@ public class ServiceLock implements Watcher {
List<String> children = validateAndSort(path,
zc.getChildren(path.toString()));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
return null;
}
@@ -682,7 +688,7 @@ public class ServiceLock implements Watcher {
List<String> children = validateAndSort(path,
zc.getChildren(path.toString()));
- if (children == null || children.isEmpty()) {
+ if (children.isEmpty()) {
return 0;
}
@@ -714,7 +720,7 @@ public class ServiceLock implements Watcher {
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 @@ public class ServiceLock implements Watcher {
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 d94c358b13..44b6fcd4ce 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 @@ package org.apache.accumulo.core.fate.zookeeper;
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 @@ public class ServiceLockTest {
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 fd71842f78..7069ed5b73 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.ServiceLock.ServiceLockPath;
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 @@ public class ServiceLockIT {
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 @@ public class ServiceLockIT {
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 @@ public class ServiceLockIT {
assertTrue(zlw2.isLockHeld());
zl2.unlock();
- zk2.close();
-
}
}
@@ -533,9 +525,8 @@ public class ServiceLockIT {
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 @@ public class ServiceLockIT {
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 @@ public class ServiceLockIT {
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 @@ public class ServiceLockIT {
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 @@ public class ServiceLockIT {
assertEquals("test2", new String(zk.getData(zl.getLockPath(), null,
null)));
}
}
-
}
