[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/136 News on this great feature? ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 re: ACL and recursive watches... ZooKeeper.exists() does not check ACLs so I assert that the current behavior of this PR doesn't violate the current behavior. i.e. in ZooKeeper you can have knowledge of the existence of every ZNode in the database irrespective of ACL. I can update the docs to make this clear however if people think that that's needed. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 One of our devs, @alexbrasetvik, asked about ACLs and recursive watches. It turns out this PR is not handling that and needs to. I'll submit support for this soon. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 This PR and #332 are ready. How can we get this merged? @skamille ? ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 As promised - here's the project with the benchmarking stuff: https://github.com/Randgalt/zkbench ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 > does the new map in the watch structure break rolling upgrades It would depend how you did it. If you upgraded all the ZK servers before adding any persistent watches you'd be OK. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user skamille commented on the issue: https://github.com/apache/zookeeper/pull/136 > Per 3. This PR does not guarantee that you will see all events. I'll double check the doc to make sure that that's clear. These watches behave exactly as other watches in ZK other than they don't remove themselves when triggered. Well, I would personally expect that a watch that doesn't remove itself when triggered is going to never drop an event so long as the session is alive. My understanding of the raft-based stores is that they can give you the historical change log that guarantees it won't lose events. I haven't done the mental exercise of determining if we could do that in ZK (probably? maybe?) but it is certainly a difference that partially arises from the underlying design differences between the systems, and we should be more specific about what will really happen with this 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user skamille commented on the issue: https://github.com/apache/zookeeper/pull/136 I want to make it clear that I'm not arguing against the feature, I just want us to make sure that we're implementing it in a way that makes sense absent the TreeSet use case, and that we at least document the performance and operational considerations of the feature. This seems important to helping people use it appropriately. When you had to do the overhead of implementing this in Curator, what did the cost in terms of memory usage/performance to track this across large numbers of 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 Another goal is feature parity with other consensus tools such as etcd/consul. I added TTL nodes with this (and other) goals earlier in the year (or was it last year?). Watches in consul are persistent and optionally recursive. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user skamille commented on the issue: https://github.com/apache/zookeeper/pull/136 We have to remember that people who don't use TreeCache will still use this feature. Not to say that we shouldn't keep it in mind as an important user, but presumably people who don't actually do anything with curator will decide to use this feature. Does the design make sense absent that consideration? Specifically, if you weren't thinking of this as a feature for TreeCache, would we implement it to automatically watch children changes as well, or would it be broken up into two modes: persistent no children, persistent 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 Per 1. I posted some performance numbers in the issue. There's a definite hit but it's worth it in my view. We should discuss this. Per 2. What this PR is aimed at is users of Curator's TreeCache - one of the most widely used "recipes" in the library. Many users want to know everything that happens to a tree of ZNodes. With the current APIs this is extraordinarily difficult (thus the complexity of the TreeCache code) and inefficient. You must set 2 watches for every single node in the tree (data and children) and then work very hard to keep those watches set as they trigger, through network issues, etc. Per 3. This PR does not guarantee that you will see all events. I'll double check the doc to make sure that that's clear. These watches behave exactly as other watches in ZK other than they don't remove themselves when triggered. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user skamille commented on the issue: https://github.com/apache/zookeeper/pull/136 Questions I have about this from a high level design perspective: 1. As I asked on the mailing list, have we done load/performance testing or addressed what that might look like in the design? (Jordan to get back to us on that) 2. I'm not sure I understand why persistent watches are both persistent and always set for all children of a node. Is it not useful to imagine that I would want a persistent watch on some node but not care about its children? Some clarification on that choice would be helpful. 3. What does it really mean to guarantee sending of all watch events? What are the implications for a disconnected client upon reconnect? How much do we expect ZK to potentially be storing in order to be able to fulfill this guarantee? Will this potentially cause unbounded memory overhead or lead to full GC? Can we realistically bound this guarantee in order to provide the other operational guarantees people expect from ZK such as generally predictable memory usage based on size of data tree? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 @afine issues addressed --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 @hanm done --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/136 This patch requires a rebase. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 @stuhood - FYI - here's a new version of Curator's cache recipes consolidated into one-recipe-to-rule-them-all that relies on this new Persistent Recursive Watch implementation. https://github.com/apache/curator/pull/181 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 Thanks @eribeiro - at minimum it would be good to get feedback. This feature will really help the community. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user eribeiro commented on the issue: https://github.com/apache/zookeeper/pull/136 Hey @fpj, @phunt, @breed, @skamille (or any PMC/commiter), it would be really cool to have this patch by @Randgalt merged, wouldn't it? If you have any cycles would you mind to review this one? Thanks!!! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 @stuhood I'll end up writing one for Apache Curator. The short answer is that large ZK users end up with 100s of thousands or millions of watchers when a few dozen would suffice. The use case is watching a tree of znodes. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user stuhood commented on the issue: https://github.com/apache/zookeeper/pull/136 Driveby comment: would be great to see a "recipe" or some real world example of correctly using this API to watch a tree. Is the goal that consumption of only the watch events would be sufficient for a client to stay synchronized for some tree? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 > it is required to add a C client change in this patch too? @eribeiro C client work is separate. I usually add a new Jira for them. If/when this patch is accepted I'll do that. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user eribeiro commented on the issue: https://github.com/apache/zookeeper/pull/136 @Randgalt it is required to add a C client change in this patch too? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user eribeiro commented on the issue: https://github.com/apache/zookeeper/pull/136 @Randgalt yup, very cool the ``asIterable()`` solution. +1. :) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #136: [ZOOKEEPER-1416] Persistent Recursive Watch
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/136 @eribeiro Have a look at the change to `PathIterator` I think this solves your desire while not breaking implicit contracts. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
