[CURATOR-351] Support for TTL Nodes #171
Conversation
| client.start(); | ||
|
|
||
| client.create().withTtl(10).creatingParentsIfNeeded().withMode(CreateMode.PERSISTENT_WITH_TTL).forPath("/a/b/c"); | ||
| Thread.sleep(20); |
There was a problem hiding this comment.
I assume that client should get a 'removed' event when the node times out and gets removed by ZK? If so, wouldn't it be less error prone to wait on a latch and checking that the node was deleted within 10 +- some fudge factor rather than just sleeping?
There was a problem hiding this comment.
I'll take a look at that.
There was a problem hiding this comment.
On reflection, I want to verify that the node is deleted within the TTL so the sleep seems appropriate to me. Right?
There was a problem hiding this comment.
You're probably right, my concern is that I don't know how quick ZK responds to these TTL expiry events. You've only got a 10ms window here for ZK to remove the node. Maybe that's enough.
There was a problem hiding this comment.
I know exactly how long because I wrote it :D Note at the top of the test class: System.setProperty("znode.container.checkIntervalMs", "1");
| @@ -83,6 +85,13 @@ public CreateBuilderMain orSetData() | |||
| return this; | |||
| } | |||
|
|
|||
| @Override | |||
| public CreateBuilderMain withTtl(long ttl) | |||
There was a problem hiding this comment.
I prefer withTTL, but that's just personal preference. I'm not sure what the standard for capitalisation of acronyms is.
There was a problem hiding this comment.
I've started using strict camel case for everything. It's easier to remember. But, it's not a big deal to me.
There was a problem hiding this comment.
+1 to strict camel case. I don't like how it looks either, but I do like that it is more consistent.
|
FYI - TTL nodes will be in the next release of ZK (3.5.3-beta). |
Randgalt
changed the title
|
ZooKeeper 3.5.3-beta has been released with TTL support so this PR is now ready. |
| try (PersistentTtlNode node = new PersistentTtlNode(client, "/test", 10, new byte[0])) | ||
| { | ||
| node.start(); | ||
| node.waitForInitialCreate(timing.session(), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
This should probably assert that the return is true?
| @@ -44,25 +45,49 @@ public void testBasic() throws Exception | |||
| { | |||
| client.start(); | |||
|
|
|||
| try (PersistentTtlNode node = new PersistentTtlNode(client, "/test", 10, new byte[0])) | |||
| try (PersistentTtlNode node = new PersistentTtlNode(client, "/test", 100, new byte[0])) | |||
| { | |||
| node.start(); | |||
| node.waitForInitialCreate(timing.session(), TimeUnit.MILLISECONDS); | |||
There was a problem hiding this comment.
This should probably assert that the return is true?
| @@ -18,8 +18,19 @@ | |||
| */ | |||
| package org.apache.curator.framework.api; | |||
|
|
|||
| public interface CreateBuilder extends | |||
| CreateBuilderMain | |||
| public interface CreateBuilder extends CreateBuilderMain | |||
There was a problem hiding this comment.
Why not extend CreateBuilder2 here? Less duplicate code/comments for withTtl
There was a problem hiding this comment.
It's because of orSetData()
There was a problem hiding this comment.
Right. That's not a problem.
public interface CreateBuilder extends CreateBuilder2 { CreateBuilder2 orSetData() }
| @@ -83,6 +85,13 @@ public CreateBuilderMain orSetData() | |||
| return this; | |||
| } | |||
|
|
|||
| @Override | |||
| public CreateBuilderMain withTtl(long ttl) | |||
There was a problem hiding this comment.
+1 to strict camel case. I don't like how it looks either, but I do like that it is more consistent.
|
|
||
| /** | ||
| * @param client the client | ||
| * @param path path for the parent ZNode |
There was a problem hiding this comment.
missing javadoc for executor service. probably worthwhile to caution people that if the executor service is overloaded then it may impact the ability to do ttl updates. we should also give users a way to realize that this is the case when their nodes start expiring. probably not a very common use case though.
Early PR for TTL nodes support so others can test.