You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by Randgalt <gi...@git.apache.org> on 2015/05/19 23:34:51 UTC
[GitHub] curator pull request: Container support
GitHub user Randgalt opened a pull request:
https://github.com/apache/curator/pull/79
Container support
Assuming that ZOOKEEPER-2163 is accepted, container node support has been added to Curator. Also, all recipes that create parent nodes now create container nodes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/curator CURATOR-222
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/79.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 #79
----
commit 04ae8115512f03210cf930118788c67e433bd904
Author: randgalt <ra...@apache.org>
Date: 2015-05-19T21:33:24Z
Assuming that ZOOKEEPER-2163 is accepted, container node support has been added to Curator. Also, all recipes that create parent nodes now create container nodes
----
---
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-222] Container support
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-117845786
Ok, all looks good as far as I'm concerned. Nice work, this looks like really good feature.
---
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-222] Container support
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-117844520
OK - I see the issue with NamespaceImpl. Fixed.
---
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-222] Container support
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-115043444
I ended up adding some more behavior and refining things. Please give a once-over if possible.
---
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-222] Container support
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-113980629
Oh right. Those are added 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-222] Container support
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/79#discussion_r33725646
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestPathChildrenCache.java ---
@@ -100,16 +100,16 @@ public void childEvent(CuratorFramework client, PathChildrenCacheEvent event) th
}
}
- @Test
+ @Test(enabled = false) // I didn't write this test and it's not working. I'm not even sure what it tests. Check into it later -JZ
--- End diff --
Yeah - I think it should be removed. I'll remove it.
---
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-222] Container support
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-115121775
Will try and have a look tomorrow, real work is pretty flat out at the moment.
---
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-222] Container support
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-103723353
Good point. In fact, we should use @since more broadly. But, I'll add this.
---
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-222] Container support
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-117380664
@Randgalt, looks good to me. A couple of minor things for discussion.
1.) Can the internal EnsurePath stuff in NamespaceImpl be replaced with the new container stuff?
2.) When executing creatingContainerParentsIfNeeded(), I was wondering if some sort of warning log should be generated if the parents already exist and are not container nodes? I'm just thinking of the case where you try and create a hierachy of nodes and some of them already exist as persistent nodes. Maybe it's just up to the user to remove any existing parents if this is important?
---
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-222] Container support
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-117838767
My point with the NamespaceImpl stuff is that it's using EnsurePath (which is now deprecated) internally, Presumably if it's deprectated then it should be replaced by the container stuff.
---
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-222] Container support
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-114592755
Any last comments before I merge?
---
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-222] Container support
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/79
---
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-222] Container support
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/79#discussion_r33639772
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestPathChildrenCache.java ---
@@ -100,16 +100,16 @@ public void childEvent(CuratorFramework client, PathChildrenCacheEvent event) th
}
}
- @Test
+ @Test(enabled = false) // I didn't write this test and it's not working. I'm not even sure what it tests. Check into it later -JZ
--- End diff --
What's the status of this? Looks like it was added as part of CURATOR-141, and I guess it's trying to get the client to shutdown during the build process. It looks pretty flaky though? Maybe we should just remove it
---
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-222] Container support
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-117826258
> Can the internal EnsurePath stuff in NamespaceImpl be replaced with the new container stuff?
Possibly, but I'm deprecating EnsurePath so I don't see the need to add to it
> When executing creatingContainerParentsIfNeeded(), I was wondering if some sort of warning log should be generated if the parents already exist and are not container nodes?
Unfortunately, there's no way to know if a node is a container or not right now. I had to shoehorn container support in with a hack. Maybe in a future version of ZK.
---
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-222] Container support
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-117846053
Thank you
---
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-222] Container support
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-113968827
I've had a brief look and it looks OK to me, though the deprecated tags still don't have "since 3.0.0" from what I can see?
---
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-222] Container support
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-116041428
Unfortunately, I'm too flat out at the moment to look at this. Later next week I'll get a chance, but the real world is just too busy at the moment sorry.
---
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-222] Container support
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/79#issuecomment-103719656
I think it would be helpful to users for `@deprecated` annotations to say "since 3.0.0"
Looks good so far, I'll give it a more thorough look tomorrow.
---
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.
---