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/09 04:06:36 UTC
[GitHub] curator pull request: CURATOR-138: Unify event listening for NodeC...
GitHub user dragonsinth opened a pull request:
https://github.com/apache/curator/pull/34
CURATOR-138: Unify event listening for NodeCache, PathChildrenCache, TreeCache
WIP. Just wanted to run the basic idea by before I go much farther down this path.
- Need tests for NodeCache
- Need to add new listener type to PathChildrenCache
- Need tests for PathChildrenCache
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dragonsinth/curator CURATOR-138
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/34.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 #34
----
commit f8ef2d9109d38133a34c99dab8362d5eef41ab99
Author: Scott Blum <sc...@squareup.com>
Date: 2014-08-09T02:04:40Z
CURATOR-138: Unify event listening for NodeCache, PathChildrenCache, TreeCache
----
---
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-138: Unify event listening for NodeC...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52510910
So I wasn't planning to re-implement NodeCache or PathChildrenCache in terms of TreeCache, but just to add the new events into the existing code, basically.
That said, with max depth of 0, TreeCache ~= NodeCache; with max depth of 1, it's ~= PathChildrenCache. So instead of adding the new event listener types to the old classes, we could just deprecate everything and double-down on TreeCache. That seems reasonable to me.
---
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-138: Rename TreeCacheEvent -> CacheE...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/34
---
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-138: Unify event listening for NodeC...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52429668
I'm actually having second thoughts about this. Maybe the best approach is to just mark the PathChildrenCache-based classes as @deprecated and not worry about unification. My fear is that there are unknown side effects of PCC that users are relying on and we will introduce bugs from unification. Thoughts?
---
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-138: Unify event listening for NodeC...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52137268
Is this the right approach, before I continue?
---
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-138: Unify event listening for NodeC...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-51721908
I wonder if we should unify TreeCache and PathChildrenCache as well.
---
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-138: Unify event listening for NodeC...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52560703
I think we're all in agreement that unifying the 3 cache implementations is the way to go. What's the way forward to getting it done? Do we just leave PathChildrenCache and NodeCache as is and depreceate them? Then provide a means for passing a 'max depth' to the TreeCache.
Then, we can just stop maintaining the 2 deprecated caches and ask people to move to TreeCache if they find issues.
It's not as clean as just reskinning the PathChildrenCache and NodeCache, but it does give the TreeCache some time to settle.
---
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-138: Rename TreeCacheEvent -> CacheE...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52836001
So, I can close this PR?
---
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-138: Unify event listening for NodeC...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52140785
I won't be able to look at this until the weekend. FYI
---
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-138: Unify event listening for NodeC...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52145665
No prob!
---
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-138: Unify event listening for NodeC...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-51722381
Yeah, that's absolutely the plan. I just wanted to make sure I was heading in the right direction before doing the work in PathChildrenCache (and writing tests).
---
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-138: Unify event listening for NodeC...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52561068
My instinct is to just deprecate the old classes. Trying to emulate any hidden side-effects that users are relying on will be non-productive.
---
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-138: Unify event listening for NodeC...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52276611
Looks good to me so far Scott. In regards to unifying TreeCache and PathChildrenCache, I definitely reckon this is the way forward. I guess we would need to implement some sort of 'max tree depth' argument to the TreeCache so that it can replicate the single level of caching that the PathChildrenCache uses currently.
---
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-138: Unify event listening for NodeC...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52511115
That's where I'm coming from. It seems redundant to have multiple implementations for the same thing. That said, NodeCache and PathChildrenCache have been heavily tested by lots of users. TreeCache will need to go through the same usage.
---
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-138: Unify event listening for NodeC...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52561576
Sounds like a plan
---
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-138: Unify event listening for NodeC...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52575711
Sounds great. I'll revert the changes to NodeCache / PathChildrenCache then and just rename TreeCacheEvent -> CacheEvent. Then I'll work on maxDepth in a separate PR. We can deprecate once maxDepth is landed.
---
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-138: Rename TreeCacheEvent -> CacheE...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/34#issuecomment-52839835
Yeah, I suppose so.
---
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.
---