Skip to content

ZOOKEEPER-3546: Delete empty containers with cversion == 0 after a grace period #1091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
swallez wants to merge 1 commit into from
Closed

ZOOKEEPER-3546: Delete empty containers with cversion == 0 after a grace period #1091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public class ContainerManager {
private final int maxPerMinute;
private final Timer timer;
private final AtomicReference<TimerTask> task = new AtomicReference<TimerTask>(null);
private final Set<String> noChildrenAtLastCheck = new HashSet<String>();

/**
* @param zkDb the ZK database
Expand Down Expand Up @@ -139,14 +140,22 @@ protected Collection<String> getCandidates() {
Set<String> candidates = new HashSet<String>();
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.stat.getCversion() > 0) && (node.getChildren().isEmpty())) {
candidates.add(containerPath);
boolean wasNewWithNoChildren = noChildrenAtLastCheck.remove(containerPath);

if (node != null && node.getChildren().isEmpty()) {
if (node.stat.getCversion() == 0) {
// Give newly created containers a grace period and avoid deleting
// them before any children could be added. If you were to create the
// container just before a container cleaning period the container
// would be immediately be deleted.
if (wasNewWithNoChildren) {
candidates.add(containerPath);
} else {
noChildrenAtLastCheck.add(containerPath);

@eolivelli eolivelli Sep 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a first glance we are only adding elements and never clearing the collection

@swallez swallez Sep 17, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do it outside of the getChildren().isEmpty() test. Children may have been added since the container was added to that collection, so we need to make sure that containers with children don't stay in it forever.

boolean wasNewWithNoChildren = noChildrenAtLastCheck.remove(containerPath);

}
} else {
candidates.add(containerPath);
}
}
}
for (String ttlPath : zkDb.getDataTree().getTtls()) {
Expand Down
16 changes: 16 additions & 0 deletions zookeeper-server/src/test/java/org/apache/zookeeper/server/CreateContainerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,22 @@ public Void call() throws Exception {
assertEquals(queue.poll(5, TimeUnit.SECONDS), "/four");
}

@Test(timeout = 30000)
public void testContainerWithNoChildGracePeriod() throws KeeperException, InterruptedException {
zk.create("/foo", new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.CONTAINER);

ContainerManager containerManager = new ContainerManager(serverFactory.getZooKeeperServer().getZKDatabase(), serverFactory.getZooKeeperServer().firstProcessor, 1, 100);
containerManager.checkContainers();
Thread.sleep(1000);

assertNotNull("Container should still be there", zk.exists("/foo", false));

containerManager.checkContainers();

@eolivelli eolivelli Sep 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this sleep?
This test may turn into a flaky one

@swallez swallez Sep 17, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the same approach as other tests in that class, where I understand that these sleep after each call to checkContainers are precisely there to avoid flakiness by giving time to the ZK server to asynchronously delete the nodes that have been selected for removal.

Did I miss something?

Thread.sleep(1000);

assertNull("Container should 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);
Expand Down