From 28f95368e7fa4190f0153452a2798fca49ec25f3 Mon Sep 17 00:00:00 2001 From: randgalt Date: Sat, 9 Nov 2019 11:36:31 -0500 Subject: [PATCH] ZOOKEEPER-3546 Edge cases can cause Container Nodes to never be deleted. i.e. if the container node is created and then the client that create the node crashes the container will not get deleted unless another client creates a node inside of it. This is because the initial implementation does not delete container nodes with a cversion of 0. This PR adds a new system property, "znode.container.maxZeroChildrenIntervalMs", that can be set to delete containers with a cversion of 0 that have been retained for a period of time. This is a backward compatible change as the default value for this is Long.MAX_VALUE - i.e. never. --- .../main/resources/markdown/zookeeperAdmin.md | 9 ++ .../zookeeper/server/ContainerManager.java | 52 ++++++++++-- .../zookeeper/server/ZooKeeperServerMain.java | 4 +- .../server/quorum/LeaderZooKeeperServer.java | 4 +- .../zookeeper/server/CreateContainerTest.java | 82 ++++++++++++++++--- 5 files changed, 129 insertions(+), 22 deletions(-) diff --git a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md index 272731c6e0f..bcf9d1c5646 100644 --- a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md +++ b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md @@ -1603,6 +1603,15 @@ Both subsystems need to have sufficient amount of threads to achieve peak read t minute. This prevents herding during container deletion. Default is "10000". +* *znode.container.maxNeverUsedIntervalMs* : + (Java system property only) + **New in 3.6.0:** The + maximum interval in milliseconds that a container that has never had + any children is retained. Should be long enough for your client to + create the container, do any needed work and then create children. + Default is "0" which is used to indicate that containers + that have never had any children are never deleted. + #### Debug Observability Configurations diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java index 81a07a6ed00..66b0240b410 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java @@ -46,6 +46,7 @@ public class ContainerManager { private final RequestProcessor requestProcessor; private final int checkIntervalMs; private final int maxPerMinute; + private final long maxNeverUsedIntervalMs; private final Timer timer; private final AtomicReference task = new AtomicReference(null); @@ -58,13 +59,28 @@ public class ContainerManager { * herding of container deletions */ public ContainerManager(ZKDatabase zkDb, RequestProcessor requestProcessor, int checkIntervalMs, int maxPerMinute) { + this(zkDb, requestProcessor, checkIntervalMs, maxPerMinute, Long.MAX_VALUE); + } + + /** + * @param zkDb the ZK database + * @param requestProcessor request processer - used to inject delete + * container requests + * @param checkIntervalMs how often to check containers in milliseconds + * @param maxPerMinute the max containers to delete per second - avoids + * herding of container deletions + * @param maxNeverUsedIntervalMs the max time in milliseconds that a container that has never had + * any children is retained + */ + public ContainerManager(ZKDatabase zkDb, RequestProcessor requestProcessor, int checkIntervalMs, int maxPerMinute, long maxNeverUsedIntervalMs) { this.zkDb = zkDb; this.requestProcessor = requestProcessor; this.checkIntervalMs = checkIntervalMs; this.maxPerMinute = maxPerMinute; + this.maxNeverUsedIntervalMs = maxNeverUsedIntervalMs; timer = new Timer("ContainerManagerTask", true); - LOG.info("Using checkIntervalMs={} maxPerMinute={}", checkIntervalMs, maxPerMinute); + LOG.info("Using checkIntervalMs={} maxPerMinute={} maxNeverUsedIntervalMs={}", checkIntervalMs, maxPerMinute, maxNeverUsedIntervalMs); } /** @@ -116,7 +132,7 @@ public void checkContainers() throws InterruptedException { Request request = new Request(null, 0, 0, ZooDefs.OpCode.deleteContainer, path, null); try { LOG.info("Attempting to delete candidate container: {}", containerPath); - requestProcessor.processRequest(request); + postDeleteRequest(request); } catch (Exception e) { LOG.error("Could not delete container: {}", containerPath, e); } @@ -129,6 +145,11 @@ public void checkContainers() throws InterruptedException { } } + // VisibleForTesting + protected void postDeleteRequest(Request request) throws RequestProcessor.RequestProcessorException { + requestProcessor.processRequest(request); + } + // VisibleForTesting protected long getMinIntervalMs() { return TimeUnit.MINUTES.toMillis(1) / maxPerMinute; @@ -139,12 +160,26 @@ protected Collection getCandidates() { Set candidates = new HashSet(); for (String containerPath : zkDb.getDataTree().getContainers()) { DataNode node = zkDb.getDataTree().getNode(containerPath); - /* - cversion > 0: keep newly created containers from being deleted - before any children have been added. If you were to create the - container just before a container cleaning period the container - would be immediately be deleted. - */ + if ((node != null) && node.getChildren().isEmpty()) { + /* + cversion > 0: keep newly created containers from being deleted + before any children have been added. If you were to create the + container just before a container cleaning period the container + would be immediately be deleted. + */ + if (node.stat.getCversion() > 0) { + candidates.add(containerPath); + } else { + /* + Users may not want unused containers to live indefinitely. Allow a system + property to be set that sets the max time for a cversion-0 container + to stay before being deleted + */ + if ((maxNeverUsedIntervalMs != 0) && (getElapsed(node) > maxNeverUsedIntervalMs)) { + candidates.add(containerPath); + } + } + } if ((node != null) && (node.stat.getCversion() > 0) && (node.getChildren().isEmpty())) { candidates.add(containerPath); } @@ -155,7 +190,6 @@ protected Collection getCandidates() { Set children = node.getChildren(); if (children.isEmpty()) { if (EphemeralType.get(node.stat.getEphemeralOwner()) == EphemeralType.TTL) { - long elapsed = getElapsed(node); long ttl = EphemeralType.TTL.getValue(node.stat.getEphemeralOwner()); if ((ttl != 0) && (getElapsed(node) > ttl)) { candidates.add(ttlPath); diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java index 4a1b6009596..3b87312a2fc 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java @@ -169,7 +169,9 @@ public void runFromConfig(ServerConfig config) throws IOException, AdminServerEx zkServer.getZKDatabase(), zkServer.firstProcessor, Integer.getInteger("znode.container.checkIntervalMs", (int) TimeUnit.MINUTES.toMillis(1)), - Integer.getInteger("znode.container.maxPerMinute", 10000)); + Integer.getInteger("znode.container.maxPerMinute", 10000), + Long.getLong("znode.container.maxNeverUsedIntervalMs", 0) + ); containerManager.start(); ZKAuditProvider.addZKStartStopAuditLog(); diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java index a0e32f7f51a..27de4519177 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java @@ -81,7 +81,9 @@ private synchronized void setupContainerManager() { getZKDatabase(), prepRequestProcessor, Integer.getInteger("znode.container.checkIntervalMs", (int) TimeUnit.MINUTES.toMillis(1)), - Integer.getInteger("znode.container.maxPerMinute", 10000)); + Integer.getInteger("znode.container.maxPerMinute", 10000), + Long.getLong("znode.container.maxNeverUsedIntervalMs", 0) + ); } @Override diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/CreateContainerTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/CreateContainerTest.java index 83c7f0b3db9..03f9bcce210 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/CreateContainerTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/CreateContainerTest.java @@ -31,7 +31,10 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; import org.apache.zookeeper.AsyncCallback; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.KeeperException; @@ -45,11 +48,24 @@ public class CreateContainerTest extends ClientBase { private ZooKeeper zk; + private Semaphore completedContainerDeletions; @Override public void setUp() throws Exception { super.setUp(); zk = createClient(); + + completedContainerDeletions = new Semaphore(0); + ZKDatabase testDatabase = new ZKDatabase(serverFactory.zkServer.getZKDatabase().snapLog) { + @Override + public void addCommittedProposal(Request request) { + super.addCommittedProposal(request); + if (request.type == ZooDefs.OpCode.deleteContainer) { + completedContainerDeletions.release(); + } + } + }; + serverFactory.zkServer.setZKDatabase(testDatabase); } @Override @@ -95,8 +111,7 @@ public void testSimpleDeletion() throws KeeperException, InterruptedException { ContainerManager containerManager = new ContainerManager(serverFactory.getZooKeeperServer().getZKDatabase(), serverFactory.getZooKeeperServer().firstProcessor, 1, 100); containerManager.checkContainers(); - Thread.sleep(1000); - + assertTrue(completedContainerDeletions.tryAcquire(1, TimeUnit.SECONDS)); assertNull("Container should have been deleted", zk.exists("/foo", false)); } @@ -123,8 +138,7 @@ public void testMultiWithContainer() throws KeeperException, InterruptedExceptio ContainerManager containerManager = new ContainerManager(serverFactory.getZooKeeperServer().getZKDatabase(), serverFactory.getZooKeeperServer().firstProcessor, 1, 100); containerManager.checkContainers(); - Thread.sleep(1000); - + assertTrue(completedContainerDeletions.tryAcquire(1, TimeUnit.SECONDS)); assertNull("Container should have been deleted", zk.exists("/foo", false)); createContainer = Op.create("/foo", new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.CONTAINER); @@ -134,8 +148,7 @@ public void testMultiWithContainer() throws KeeperException, InterruptedExceptio containerManager.checkContainers(); - Thread.sleep(1000); - + assertTrue(completedContainerDeletions.tryAcquire(1, TimeUnit.SECONDS)); assertNull("Container should have been deleted", zk.exists("/foo", false)); } @@ -157,8 +170,7 @@ public void processResult(int rc, String path, Object ctx, String name, Stat sta ContainerManager containerManager = new ContainerManager(serverFactory.getZooKeeperServer().getZKDatabase(), serverFactory.getZooKeeperServer().firstProcessor, 1, 100); containerManager.checkContainers(); - Thread.sleep(1000); - + assertTrue(completedContainerDeletions.tryAcquire(1, TimeUnit.SECONDS)); assertNull("Container should have been deleted", zk.exists("/foo", false)); } @@ -171,9 +183,9 @@ public void testCascadingDeletion() throws KeeperException, InterruptedException ContainerManager containerManager = new ContainerManager(serverFactory.getZooKeeperServer().getZKDatabase(), serverFactory.getZooKeeperServer().firstProcessor, 1, 100); containerManager.checkContainers(); - Thread.sleep(1000); + assertTrue(completedContainerDeletions.tryAcquire(1, TimeUnit.SECONDS)); containerManager.checkContainers(); - Thread.sleep(1000); + assertTrue(completedContainerDeletions.tryAcquire(1, TimeUnit.SECONDS)); assertNull("Container should have been deleted", zk.exists("/foo/bar", false)); assertNull("Container should have been deleted", zk.exists("/foo", false)); @@ -191,8 +203,8 @@ protected Collection getCandidates() { } }; containerManager.checkContainers(); - Thread.sleep(1000); + assertTrue(completedContainerDeletions.tryAcquire(1, TimeUnit.SECONDS)); assertNotNull("Container should have not been deleted", zk.exists("/foo", false)); } @@ -237,6 +249,54 @@ public Void call() throws Exception { assertEquals(queue.poll(5, TimeUnit.SECONDS), "/four"); } + @Test(timeout = 30000) + public void testMaxNeverUsedInterval() throws KeeperException, InterruptedException { + zk.create("/foo", new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.CONTAINER); + AtomicLong elapsed = new AtomicLong(0); + AtomicInteger deletesQty = new AtomicInteger(0); + ContainerManager containerManager = new ContainerManager(serverFactory.getZooKeeperServer().getZKDatabase(), serverFactory.getZooKeeperServer().firstProcessor, 1, 100, 1000) { + @Override + protected void postDeleteRequest(Request request) throws RequestProcessor.RequestProcessorException { + deletesQty.incrementAndGet(); + super.postDeleteRequest(request); + } + + @Override + protected long getElapsed(DataNode node) { + return elapsed.get(); + } + }; + containerManager.checkContainers(); // elapsed time will appear to be 0 - container will not get deleted + assertEquals(deletesQty.get(), 0); + assertNotNull("Container should not have been deleted", zk.exists("/foo", false)); + + elapsed.set(10000); + containerManager.checkContainers(); // elapsed time will appear to be 10000 - container should get deleted + assertTrue(completedContainerDeletions.tryAcquire(1, TimeUnit.SECONDS)); + assertNull("Container should have been deleted", zk.exists("/foo", false)); + } + + @Test(timeout = 30000) + public void testZeroMaxNeverUsedInterval() throws KeeperException, InterruptedException { + zk.create("/foo", new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.CONTAINER); + AtomicInteger deletesQty = new AtomicInteger(0); + ContainerManager containerManager = new ContainerManager(serverFactory.getZooKeeperServer().getZKDatabase(), serverFactory.getZooKeeperServer().firstProcessor, 1, 100, 0) { + @Override + protected void postDeleteRequest(Request request) throws RequestProcessor.RequestProcessorException { + deletesQty.incrementAndGet(); + super.postDeleteRequest(request); + } + + @Override + protected long getElapsed(DataNode node) { + return 10000; // some number greater than 0 + } + }; + containerManager.checkContainers(); // elapsed time will appear to be 0 - container will not get deleted + assertEquals(deletesQty.get(), 0); + assertNotNull("Container should not have been deleted", zk.exists("/foo", false)); + } + private void createNoStatVerifyResult(String newName) throws KeeperException, InterruptedException { assertNull("Node existed before created", zk.exists(newName, false)); zk.create(newName, newName.getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.CONTAINER);