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

[swallez]

Ensures empty containers that never had children added are deleted after a grace period.

The grace period ensures that such empty containers live at for one full collection period. This gives clients time to create the container and add children in separate transactions even if the container is created right before the collection task is run.

[maoling]

• If I create a container node which has not any children yet, it will never be deleted by the cleaning task, this behavior is expected.
• the doc in the zookeeperProgrammers.md

ZooKeeper has the notion of container znodes. Container znodes are
special purpose znodes useful for recipes such as leader, lock, etc.
When the last child of a container is deleted, the container becomes
a candidate to be deleted by the server at some point in the future.
Given this property, you should be prepared to get
KeeperException.NoNodeException when creating children inside of
container znodes. i.e. when creating child znodes inside of container znodes
always check for KeeperException.NoNodeException and recreate the container
znode when it occurs.

[eolivelli]

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

[swallez]

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);

[eolivelli]

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

[swallez]

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?

[tisonkun]

I second @maoling here that "If I create a container node which has not any children yet, it will never be deleted by the cleaning task, this behavior is expected."

When a user create a container node he must prepare to create several children of it. Otherwise it is a misuse that the user should take care of the clean-up by himself. In an edge case that the creation of child fails, it is the responsibility of users to tolerate the failing case.

Mainly my concern is if we introduce such grace period it becomes even worse users are now enforced to take care of an edge case caused by ZooKeeper: if they create a container first, following a creation of child which suffer network traffic and fails because of the grace period expire.

Besides, I'm afraid it also affects how downstream project like Curator takes care of container znodes. cc @Randgalt

[maoling]

• this change has violated the semantic of CONTAINER node designed from ZOOKEEPER-2163,
  making the behavior of CONTAINER node very weird.

This proposal adds a new node type, CONTAINER. A CONTAINER node is the same as a PERSISTENT node with the additional property that when its last child is deleted, it is deleted (and CONTAINER nodes recursively up the tree are deleted if empty).

• Further:the exist of CONTAINER node is an important flag for the developers to implement their recipes.how can I distinguish the Non-existent of that CONTAINER node is because of all her children have been deleted or she has no children at all.
• What a scene I can image is: I have a main task which has many sub-tasks, then I can create a container node /main-task, then create ephemeral sequence nodes for every sub-tasks(e.g:/main-task/sub-task1,/main-task/sub-task2), a sub-task deleted its node after finishing its task.the main task will observer the existent of /main-task to judge whether the main task has finished successfully. If we pick up this patch, if all the sub-threads have all failed, after some time, the main task will also observer the in-existent of /main-task, mistakenly consider the state of main-task.

[nkalmar]

I also agree with @maoling and @tisonkun .
For me, it just makes more sense that an initialized container node does not get deleted. I wouldn't expect this to happen.

I personally also don't like the unconfigurable collection period TTL.

This is a behavioural change rather than a bug. If you want to change the behaviour, I think it should be first discussed with the community on [email protected]

[swallez]

Ok, I understand this is a feature rather than an unwanted side effect of giving time for application to create childrens, and I won't dispute that.

What led to proposing this change is a problem we encountered when using Curator's PersistentTtlNode that creates a container and adds a child to it, but doesn't do it as a transaction. This led to the issue we encountered.

I'm therefore closing this PR and the associated issue and will address this issue in Curator.

[tisonkun]

Thanks for your update @swallez . Sounds interesting what Curator's PersistentTtlNode does. Will check it out :-)

[Randgalt]

Sorry - I missed the cc on this. I'll follow on Curator if @swallez opens an issue there.

[swallez]

@tisonkun PersistentTtlNode creates a container as a PersistentNode and then adds a TTL node in it named touch that is refreshed periodically. This means the application can use the parent as a regular node and won't be polluted by watch event triggered by the periodic refresh required by the TTL.

The problem in Curator is that those two nodes (container and touch child) are created separately. So if there's a crash between these two requests and touch is never created, the parent container stays forever.

[swallez]

@Randgalt creating an issue on Curator right now ;-)

[Randgalt]

Yeah - I think it can be done in a transaction. The transaction APIs support TTL I'm pretty sure.

[henrikno]

Curator issue for reference: https://issues.apache.org/jira/browse/CURATOR-545