You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by dragonsinth <gi...@git.apache.org> on 2014/08/22 22:36:30 UTC
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
GitHub user dragonsinth opened a pull request:
https://github.com/apache/curator/pull/42
CURATOR-145: TreeCache should implement maxDepth
Depends on CURATOR-144
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dragonsinth/curator CURATOR-145
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/42.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #42
----
commit a8ff472cb0c5e63b7f87a9ebfd22c2c9660d55e5
Author: Scott Blum <sc...@squareup.com>
Date: 2014-08-22T20:00:20Z
CURATOR-144: TreeCache.Builder to configure advanced options
commit cdda891614e792bc6a794a81805a3ffd37042f36
Author: Scott Blum <sc...@squareup.com>
Date: 2014-08-22T20:34:29Z
CURATOR-145: TreeCache should implement maxDepth
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by jhump <gi...@git.apache.org>.
Github user jhump commented on a diff in the pull request:
https://github.com/apache/curator/pull/42#discussion_r19055906
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -179,24 +191,34 @@ public static Builder newBuilder(CuratorFramework client, String path)
final AtomicReference<Stat> stat = new AtomicReference<Stat>();
final AtomicReference<byte[]> data = new AtomicReference<byte[]>();
final AtomicReference<ConcurrentMap<String, TreeNode>> children = new AtomicReference<ConcurrentMap<String, TreeNode>>();
+ final int depth;
TreeNode(String path, TreeNode parent)
{
this.path = path;
this.parent = parent;
+ this.depth = parent == null ? 0 : parent.depth + 1;
}
private void refresh() throws Exception
{
- outstandingOps.addAndGet(2);
- doRefreshData();
- doRefreshChildren();
+ if (depth < maxDepth)
--- End diff --
I know this wasn't the intent of this PR. But to *really* solve an even larger class of problems, we can make this more flexible. What I'm thinking, instead of hard-coded criteria by depth, is the ability to provide a `Predicate` for filtering parts of the sub-tree that don't need to be cached.
Ideally, the predicate would be applied in a different spot than here. That way we can filter individual nodes out of the cache. (This approach only lets me say "yes, include all of this node's children" or "no, none of its children".)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/42#discussion_r19109606
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java ---
@@ -68,6 +68,53 @@ public void testStartEmpty() throws Exception
}
@Test
+ public void testDepth0() throws Exception
+ {
+ client.create().forPath("/test");
+ client.create().forPath("/test/1", "one".getBytes());
+ client.create().forPath("/test/2", "two".getBytes());
+ client.create().forPath("/test/3", "three".getBytes());
+ client.create().forPath("/test/2/sub", "two-sub".getBytes());
+
+ cache = buildWithListeners(TreeCache.newBuilder(client, "/test").setMaxDepth(0));
+ cache.start();
+ assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test");
+ assertEvent(TreeCacheEvent.Type.INITIALIZED);
+ assertNoMoreEvents();
+
+ Assert.assertEquals(cache.getCurrentChildren("/test").keySet(), ImmutableSet.of());
+ Assert.assertNull(cache.getCurrentData("/test/1"));
+ Assert.assertNull(cache.getCurrentChildren("/test/1"));
+ Assert.assertNull(cache.getCurrentData("/test/non_exist"));
+ }
+
+ @Test
+ public void testDepth1() throws Exception
+ {
+ client.create().forPath("/test");
+ client.create().forPath("/test/1", "one".getBytes());
+ client.create().forPath("/test/2", "two".getBytes());
+ client.create().forPath("/test/3", "three".getBytes());
+ client.create().forPath("/test/2/sub", "two-sub".getBytes());
+
+ cache = buildWithListeners(TreeCache.newBuilder(client, "/test").setMaxDepth(1));
--- End diff --
added
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/42
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/42#discussion_r19093513
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -179,24 +191,34 @@ public static Builder newBuilder(CuratorFramework client, String path)
final AtomicReference<Stat> stat = new AtomicReference<Stat>();
final AtomicReference<byte[]> data = new AtomicReference<byte[]>();
final AtomicReference<ConcurrentMap<String, TreeNode>> children = new AtomicReference<ConcurrentMap<String, TreeNode>>();
+ final int depth;
TreeNode(String path, TreeNode parent)
{
this.path = path;
this.parent = parent;
+ this.depth = parent == null ? 0 : parent.depth + 1;
}
private void refresh() throws Exception
{
- outstandingOps.addAndGet(2);
- doRefreshData();
- doRefreshChildren();
+ if (depth < maxDepth)
--- End diff --
This is an interesting idea. We could leave the integer based maxDepth in the builder but have it create a Predicate internally. Then, have an alternate method in the builder that takes an actual predicate. But, let's let @dragonsinth decide.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/42#discussion_r19108936
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -146,6 +147,17 @@ public Builder setExecutor(CloseableExecutorService executorService)
this.executorService = checkNotNull(executorService);
return this;
}
+
+ /**
+ * Sets the maximum depth to explore/watch. A {@code maxDepth} of {@code 0} will watch only
+ * the root node; a {@code maxDepth} of {@code 1} will watch the root node and its immediate
+ * children. Default: {@code Integer.MAX_VALUE}
+ */
+ public Builder setMaxDepth(int maxDepth)
--- End diff --
Clarified the doc a little bit here to note that maxDepth = 0 is like NodeCache, and maxDepth = 1 is kind of like PathChildrenCache. I don't think adding constants really adds enough clarity and value to justify more surface area.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/42#issuecomment-59829735
I'm inclined to want to go with what we have and reserve Predicate as a future feature. I don't think anything here so far would make it harder / preclude adding it later.
I have 2 main concerns over adding general Predicates that make me want to consider this much more carefully.
1) Running the predicate means passing control to user code on cache's event callback / mutation threads.
2) An inconsistent predicate would be super weird and possibly corrupt internal state. What would happen if a predicate's behavior changed on the fly, or gave inconsistent answers for the same node?
The idea definitely has merit I'm just not really ready to take it on right now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/42#issuecomment-59783898
Other than my minor comments, LGTM. Just decide on the Predicate idea.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/42#discussion_r19093393
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -146,6 +147,17 @@ public Builder setExecutor(CloseableExecutorService executorService)
this.executorService = checkNotNull(executorService);
return this;
}
+
+ /**
+ * Sets the maximum depth to explore/watch. A {@code maxDepth} of {@code 0} will watch only
+ * the root node; a {@code maxDepth} of {@code 1} will watch the root node and its immediate
+ * children. Default: {@code Integer.MAX_VALUE}
+ */
+ public Builder setMaxDepth(int maxDepth)
--- End diff --
Might be nice to have a constant for 0 that means one level or even a specialized method for setOneLevel().
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/42#issuecomment-59430672
This PR is now ready for review.
CC: @cconroy
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-145: TreeCache should implement maxD...
Posted by cconroy <gi...@git.apache.org>.
Github user cconroy commented on a diff in the pull request:
https://github.com/apache/curator/pull/42#discussion_r19000667
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java ---
@@ -68,6 +68,53 @@ public void testStartEmpty() throws Exception
}
@Test
+ public void testDepth0() throws Exception
+ {
+ client.create().forPath("/test");
+ client.create().forPath("/test/1", "one".getBytes());
+ client.create().forPath("/test/2", "two".getBytes());
+ client.create().forPath("/test/3", "three".getBytes());
+ client.create().forPath("/test/2/sub", "two-sub".getBytes());
+
+ cache = buildWithListeners(TreeCache.newBuilder(client, "/test").setMaxDepth(0));
+ cache.start();
+ assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test");
+ assertEvent(TreeCacheEvent.Type.INITIALIZED);
+ assertNoMoreEvents();
+
+ Assert.assertEquals(cache.getCurrentChildren("/test").keySet(), ImmutableSet.of());
+ Assert.assertNull(cache.getCurrentData("/test/1"));
+ Assert.assertNull(cache.getCurrentChildren("/test/1"));
+ Assert.assertNull(cache.getCurrentData("/test/non_exist"));
+ }
+
+ @Test
+ public void testDepth1() throws Exception
+ {
+ client.create().forPath("/test");
+ client.create().forPath("/test/1", "one".getBytes());
+ client.create().forPath("/test/2", "two".getBytes());
+ client.create().forPath("/test/3", "three".getBytes());
+ client.create().forPath("/test/2/sub", "two-sub".getBytes());
+
+ cache = buildWithListeners(TreeCache.newBuilder(client, "/test").setMaxDepth(1));
--- End diff --
i'd like to see a test that exercises max depth from a starting point deeper than the root or first node in the tree to ensure the null parent logic plays correctly there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---