[jira] Commented: (ZOOKEEPER-775) A large scale pub/sub system
[ https://issues.apache.org/jira/browse/ZOOKEEPER-775?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12897194#action_12897194 ] Ivan Kelly commented on ZOOKEEPER-775: -- I've removed ltmain.sh from the svn repo. The patch builds fine on trunk, but I did have some problems running the tests which I resolved by upping the file descriptor limits. testOwnershipChange(org.apache.hedwig.server.topics.TestZkTopicManager) seems to be failing consistently. C++ client builds. Some work still needs to be done to make testing it easier (requires a manual start of a server control daemon currently). These are fairly minor things, and I think it's more important to get hedwig into trunk. So... +1 A large scale pub/sub system Key: ZOOKEEPER-775 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-775 Project: Zookeeper Issue Type: New Feature Components: contrib Reporter: Benjamin Reed Assignee: Benjamin Reed Fix For: 3.4.0 Attachments: libs.zip, libs_2.zip, ZOOKEEPER-775.patch, ZOOKEEPER-775.patch, ZOOKEEPER-775.patch, ZOOKEEPER-775_2.patch, ZOOKEEPER-775_3.patch we have developed a large scale pub/sub system based on ZooKeeper and BookKeeper. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
High WTF count in ZooKeeper client code
Hi, I started yesterday to work on my idea of an alternative ZooKeeper client interface.[1] Instead of methods on a ZooKeeper class, a user should instantiate an Operation (Create, Delete, ...) and forward it to an Executor which handles session loss errors and alikes. By doing that, I got shocked by the sheer number of WTF issues I found. I'm sorry for ranting now, but it gets quicker to the poing. - Hostlist as string The hostlist is parsed in the ctor of ClientCnxn. This violates the rule of not doing (too much) work in a ctor. Instead the ClientCnxn should receive an object of class HostSet. HostSet could then be instantiated e.g. with a comma separated string. - cyclic dependency ClientCnxn, ZooKeeper ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. - Chroot is an attribute of ClientCnxn I'd like to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So I've components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. - deleteRecursive does not belong to the other methods DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? - massive code duplication in zookeeper class Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. - stat is returned by parameter Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. - stat calls static method on org.apache.zookeeper.server.DataTree It's a huge jump from client code to the internal server class DataTree. Shouldn't there rather be some class related to the protobuffer stat class that knows how to copy a stat? - Session class? Maybe it'd make sense to combine hostlist, sessionId, sessionPassword and sessionTimeout in a Session class so that the ctor of ClientCnxn won't get too long? I may have missed some items. :-) Once again, please excuse my harsh tone. May I put the above issues in jira and would you accept (backwards compatible) patches for it for 3.4.0? Zookeeper is a fascinating project. Cudos to the devs. I've only looked in the client side code, which is what most users of zookeeper will ever see if they see any zookeeper internal code at all. So it may make sense to make this piece of the project as nice and clean as possible. [1] http://mail-archives.apache.org/mod_mbox/hadoop-zookeeper- dev/201005.mbox/%3c201005261509.54236.tho...@koch.ro%3e Best regards, Thomas Koch, http://www.koch.ro
Re: High WTF count in ZooKeeper client code
HI Thomas, I read through the list of issues you posted, most of them seem reasonable to fix. The one's you have mentioned below might take quite a bit of time to fix and again a lot of testing! (just a warning :)). It would be great if you'd want to clean this up for 3.4. Please go ahead and file a jira. These improvements would be good to have in the zookeeper java client. For deleteRecursive, I definitely agree that it should be a helper class. I don't believe it should be in the direct zookeeper api! Thanks mahadev On 8/11/10 2:45 AM, Thomas Koch tho...@koch.ro wrote: Hi, I started yesterday to work on my idea of an alternative ZooKeeper client interface.[1] Instead of methods on a ZooKeeper class, a user should instantiate an Operation (Create, Delete, ...) and forward it to an Executor which handles session loss errors and alikes. By doing that, I got shocked by the sheer number of WTF issues I found. I'm sorry for ranting now, but it gets quicker to the poing. - Hostlist as string The hostlist is parsed in the ctor of ClientCnxn. This violates the rule of not doing (too much) work in a ctor. Instead the ClientCnxn should receive an object of class HostSet. HostSet could then be instantiated e.g. with a comma separated string. - cyclic dependency ClientCnxn, ZooKeeper ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. - Chroot is an attribute of ClientCnxn I'd like to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So I've components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. - deleteRecursive does not belong to the other methods DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? - massive code duplication in zookeeper class Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. - stat is returned by parameter Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. - stat calls static method on org.apache.zookeeper.server.DataTree It's a huge jump from client code to the internal server class DataTree. Shouldn't there rather be some class related to the protobuffer stat class that knows how to copy a stat? - Session class? Maybe it'd make sense to combine hostlist, sessionId, sessionPassword and sessionTimeout in a Session class so that the ctor of ClientCnxn won't get too long? I may have missed some items. :-) Once again, please excuse my harsh tone. May I put the above issues in jira and would you accept (backwards compatible) patches for it for 3.4.0? Zookeeper is a fascinating project. Cudos to the devs. I've only looked in the client side code, which is what most users of zookeeper will ever see if they see any zookeeper internal code at all. So it may make sense to make this piece of the project as nice and clean as possible. [1] http://mail-archives.apache.org/mod_mbox/hadoop-zookeeper- dev/201005.mbox/%3c201005261509.54236.tho...@koch.ro%3e Best regards, Thomas Koch, http://www.koch.ro
[jira] Created: (ZOOKEEPER-835) Refactoring Zookeeper Client Code
Refactoring Zookeeper Client Code - Key: ZOOKEEPER-835 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-835 Project: Zookeeper Issue Type: Improvement Components: java client Affects Versions: 3.3.1 Reporter: Patrick Datko Thomas Koch asked me to fill individual issues for the points raised in his mail to zookeeper-dev: [Mail of Thomas Koch| http://mail-archives.apache.org/mod_mbox/hadoop-zookeeper-dev/201008.mbox/%3c20100845.17507.tho...@koch.ro%3e ] He published several issues, which are present in the current zookeeper client, so a refactoring of the code would be an facility for other developers working with zookeeper. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-837) cyclic dependency ClientCnxn, ZooKeeper
[ https://issues.apache.org/jira/browse/ZOOKEEPER-837?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Patrick Datko updated ZOOKEEPER-837: Tags: cyclic dependency Affects Version/s: 3.3.1 Description: ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. cyclic dependency ClientCnxn, ZooKeeper --- Key: ZOOKEEPER-837 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-837 Project: Zookeeper Issue Type: Sub-task Affects Versions: 3.3.1 Reporter: Patrick Datko ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
Re: High WTF count in ZooKeeper client code
Also, I am assuming you have backwards compatability in mind when you suggest these changes right? The interfaces of zookeeper client should not be changing as part of this, though the recursive delete hasn't been introduced yet (its only available in 3.4, so we can move it out into a helper class). Thanks mahadev On 8/11/10 7:40 AM, Mahadev Konar maha...@yahoo-inc.com wrote: HI Thomas, I read through the list of issues you posted, most of them seem reasonable to fix. The one's you have mentioned below might take quite a bit of time to fix and again a lot of testing! (just a warning :)). It would be great if you'd want to clean this up for 3.4. Please go ahead and file a jira. These improvements would be good to have in the zookeeper java client. For deleteRecursive, I definitely agree that it should be a helper class. I don't believe it should be in the direct zookeeper api! Thanks mahadev On 8/11/10 2:45 AM, Thomas Koch tho...@koch.ro wrote: Hi, I started yesterday to work on my idea of an alternative ZooKeeper client interface.[1] Instead of methods on a ZooKeeper class, a user should instantiate an Operation (Create, Delete, ...) and forward it to an Executor which handles session loss errors and alikes. By doing that, I got shocked by the sheer number of WTF issues I found. I'm sorry for ranting now, but it gets quicker to the poing. - Hostlist as string The hostlist is parsed in the ctor of ClientCnxn. This violates the rule of not doing (too much) work in a ctor. Instead the ClientCnxn should receive an object of class HostSet. HostSet could then be instantiated e.g. with a comma separated string. - cyclic dependency ClientCnxn, ZooKeeper ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. - Chroot is an attribute of ClientCnxn I'd like to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So I've components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. - deleteRecursive does not belong to the other methods DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? - massive code duplication in zookeeper class Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. - stat is returned by parameter Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. - stat calls static method on org.apache.zookeeper.server.DataTree It's a huge jump from client code to the internal server class DataTree. Shouldn't there rather be some class related to the protobuffer stat class that knows how to copy a stat? - Session class? Maybe it'd make sense to combine hostlist, sessionId, sessionPassword and sessionTimeout in a Session class so that the ctor of ClientCnxn won't get too long? I may have missed some items. :-) Once again, please excuse my harsh tone. May I put the above issues in jira and would you accept (backwards compatible) patches for it for 3.4.0? Zookeeper is a fascinating project. Cudos to the devs. I've only looked in the client side code, which is what most users of zookeeper will ever see if they see any zookeeper internal code at all. So it may make sense to make this piece of the project as nice and clean as possible. [1] http://mail-archives.apache.org/mod_mbox/hadoop-zookeeper- dev/201005.mbox/%3c201005261509.54236.tho...@koch.ro%3e Best regards, Thomas Koch, http://www.koch.ro
[jira] Created: (ZOOKEEPER-838) Chroot is an attribute of ClientCnxn
Chroot is an attribute of ClientCnxn Key: ZOOKEEPER-838 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-838 Project: Zookeeper Issue Type: Sub-task Reporter: Patrick Datko -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-838) Chroot is an attribute of ClientCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-838?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Patrick Datko updated ZOOKEEPER-838: Tags: chroot Description: It would be better to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So there are components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. Chroot is an attribute of ClientCnxn Key: ZOOKEEPER-838 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-838 Project: Zookeeper Issue Type: Sub-task Reporter: Patrick Datko It would be better to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So there are components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Created: (ZOOKEEPER-839) deleteRecursive does not belong to the other methods
deleteRecursive does not belong to the other methods Key: ZOOKEEPER-839 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-839 Project: Zookeeper Issue Type: Sub-task Reporter: Patrick Datko -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Created: (ZOOKEEPER-840) massive code duplication in zookeeper class
massive code duplication in zookeeper class --- Key: ZOOKEEPER-840 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-840 Project: Zookeeper Issue Type: Sub-task Reporter: Patrick Datko -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-839) deleteRecursive does not belong to the other methods
[ https://issues.apache.org/jira/browse/ZOOKEEPER-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Patrick Datko updated ZOOKEEPER-839: Tags: atomic operations Affects Version/s: 3.3.1 Description: DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? deleteRecursive does not belong to the other methods Key: ZOOKEEPER-839 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-839 Project: Zookeeper Issue Type: Sub-task Affects Versions: 3.3.1 Reporter: Patrick Datko DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Created: (ZOOKEEPER-841) stat is returned by parameter
stat is returned by parameter - Key: ZOOKEEPER-841 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-841 Project: Zookeeper Issue Type: Sub-task Reporter: Patrick Datko -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-840) massive code duplication in zookeeper class
[ https://issues.apache.org/jira/browse/ZOOKEEPER-840?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Patrick Datko updated ZOOKEEPER-840: Tags: code duplication Description: Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. massive code duplication in zookeeper class --- Key: ZOOKEEPER-840 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-840 Project: Zookeeper Issue Type: Sub-task Reporter: Patrick Datko Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-841) stat is returned by parameter
[ https://issues.apache.org/jira/browse/ZOOKEEPER-841?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Patrick Datko updated ZOOKEEPER-841: Affects Version/s: 3.3.1 Description: Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. Component/s: java client stat is returned by parameter - Key: ZOOKEEPER-841 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-841 Project: Zookeeper Issue Type: Sub-task Components: java client Affects Versions: 3.3.1 Reporter: Patrick Datko Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
Re: High WTF count in ZooKeeper client code
Hallo Mahadev, thank you for your nice answer. Yes, we'll of cause preserve compatibility. Otherwise there is no chance to get accepted. I assume the following things must keep their interfaces: ZooKeeper (It'll call the new interface in the background), ASyncCallback, Watcher We may want to change: ClientCnxn (faktor out some things, remove dep on ZooKeeper) I think other classes should not be involved at all in our issues. My collegue Patrick was so kind to fill the jira issues. Best regards, Thomas Mahadev Konar: Also, I am assuming you have backwards compatability in mind when you suggest these changes right? The interfaces of zookeeper client should not be changing as part of this, though the recursive delete hasn't been introduced yet (its only available in 3.4, so we can move it out into a helper class). Thanks mahadev On 8/11/10 7:40 AM, Mahadev Konar maha...@yahoo-inc.com wrote: HI Thomas, I read through the list of issues you posted, most of them seem reasonable to fix. The one's you have mentioned below might take quite a bit of time to fix and again a lot of testing! (just a warning :)). It would be great if you'd want to clean this up for 3.4. Please go ahead and file a jira. These improvements would be good to have in the zookeeper java client. For deleteRecursive, I definitely agree that it should be a helper class. I don't believe it should be in the direct zookeeper api! Thanks mahadev On 8/11/10 2:45 AM, Thomas Koch tho...@koch.ro wrote: Hi, I started yesterday to work on my idea of an alternative ZooKeeper client interface.[1] Instead of methods on a ZooKeeper class, a user should instantiate an Operation (Create, Delete, ...) and forward it to an Executor which handles session loss errors and alikes. By doing that, I got shocked by the sheer number of WTF issues I found. I'm sorry for ranting now, but it gets quicker to the poing. - Hostlist as string The hostlist is parsed in the ctor of ClientCnxn. This violates the rule of not doing (too much) work in a ctor. Instead the ClientCnxn should receive an object of class HostSet. HostSet could then be instantiated e.g. with a comma separated string. - cyclic dependency ClientCnxn, ZooKeeper ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. - Chroot is an attribute of ClientCnxn I'd like to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So I've components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. - deleteRecursive does not belong to the other methods DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? - massive code duplication in zookeeper class Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. - stat is returned by parameter Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. - stat calls static method on org.apache.zookeeper.server.DataTree It's a huge jump from client code to the internal server class DataTree. Shouldn't there rather be some class related to the protobuffer stat class that knows how to copy a stat? - Session class? Maybe it'd make sense to combine hostlist, sessionId, sessionPassword and sessionTimeout in a Session class so that the ctor of ClientCnxn won't get too long? I may have missed some items. :-) Once again, please excuse my harsh tone. May I put the above issues in jira and would you accept
[jira] Assigned: (ZOOKEEPER-835) Refactoring Zookeeper Client Code
[ https://issues.apache.org/jira/browse/ZOOKEEPER-835?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Thomas Koch reassigned ZOOKEEPER-835: - Assignee: Thomas Koch Refactoring Zookeeper Client Code - Key: ZOOKEEPER-835 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-835 Project: Zookeeper Issue Type: Improvement Components: java client Affects Versions: 3.3.1 Reporter: Patrick Datko Assignee: Thomas Koch Thomas Koch asked me to fill individual issues for the points raised in his mail to zookeeper-dev: [Mail of Thomas Koch| http://mail-archives.apache.org/mod_mbox/hadoop-zookeeper-dev/201008.mbox/%3c20100845.17507.tho...@koch.ro%3e ] He published several issues, which are present in the current zookeeper client, so a refactoring of the code would be an facility for other developers working with zookeeper. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Created: (ZOOKEEPER-842) stat calls static method on org.apache.zookeeper.server.DataTree
stat calls static method on org.apache.zookeeper.server.DataTree Key: ZOOKEEPER-842 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-842 Project: Zookeeper Issue Type: Sub-task Reporter: Patrick Datko -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Created: (ZOOKEEPER-843) Session class?
Session class? -- Key: ZOOKEEPER-843 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-843 Project: Zookeeper Issue Type: Sub-task Reporter: Patrick Datko -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-843) Session class?
[ https://issues.apache.org/jira/browse/ZOOKEEPER-843?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Patrick Datko updated ZOOKEEPER-843: Tags: session, session class, refactored class Affects Version/s: 3.3.1 Description: Maybe it'd make sense to combine hostlist, sessionId, sessionPassword and sessionTimeout in a Session class so that the ctor of ClientCnxn won't get too long? Component/s: java client Session class? -- Key: ZOOKEEPER-843 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-843 Project: Zookeeper Issue Type: Sub-task Components: java client Affects Versions: 3.3.1 Reporter: Patrick Datko Maybe it'd make sense to combine hostlist, sessionId, sessionPassword and sessionTimeout in a Session class so that the ctor of ClientCnxn won't get too long? -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-304) factor out common methods from zookeeper.java
[ https://issues.apache.org/jira/browse/ZOOKEEPER-304?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12897316#action_12897316 ] Thomas Koch commented on ZOOKEEPER-304: --- Could you please list the common methods you're referring to. I don't know the server code but want to know, if I may keep this issue in my HEAD when working on ZOOKEEPER-835. If you think it relates, please link this issue to ZOOKEEPER-835 factor out common methods from zookeeper.java - Key: ZOOKEEPER-304 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-304 Project: Zookeeper Issue Type: Improvement Components: java client, server Affects Versions: 3.1.0 Reporter: Mahadev konar Fix For: 3.4.0 we need to factor out common methods from zookeeper.java to a commons directory for zookeeper so that it can be used both in client and server without each of them depending on each other. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
Re: High WTF count in ZooKeeper client code
wrt defining interface stability we should adopt something like hadoop is now doing: https://issues.apache.org/jira/browse/HADOOP-5073 Mahadev, do you know if this is documented somewhere? final documentation, rather than the running commentary thats on this jira? We could adopt something similar/same. Can you create a jira for that? Patrick On 08/11/2010 08:23 AM, Thomas Koch wrote: Hallo Mahadev, thank you for your nice answer. Yes, we'll of cause preserve compatibility. Otherwise there is no chance to get accepted. I assume the following things must keep their interfaces: ZooKeeper (It'll call the new interface in the background), ASyncCallback, Watcher We may want to change: ClientCnxn (faktor out some things, remove dep on ZooKeeper) I think other classes should not be involved at all in our issues. My collegue Patrick was so kind to fill the jira issues. Best regards, Thomas Mahadev Konar: Also, I am assuming you have backwards compatability in mind when you suggest these changes right? The interfaces of zookeeper client should not be changing as part of this, though the recursive delete hasn't been introduced yet (its only available in 3.4, so we can move it out into a helper class). Thanks mahadev On 8/11/10 7:40 AM, Mahadev Konarmaha...@yahoo-inc.com wrote: HI Thomas, I read through the list of issues you posted, most of them seem reasonable to fix. The one's you have mentioned below might take quite a bit of time to fix and again a lot of testing! (just a warning :)). It would be great if you'd want to clean this up for 3.4. Please go ahead and file a jira. These improvements would be good to have in the zookeeper java client. For deleteRecursive, I definitely agree that it should be a helper class. I don't believe it should be in the direct zookeeper api! Thanks mahadev On 8/11/10 2:45 AM, Thomas Kochtho...@koch.ro wrote: Hi, I started yesterday to work on my idea of an alternative ZooKeeper client interface.[1] Instead of methods on a ZooKeeper class, a user should instantiate an Operation (Create, Delete, ...) and forward it to an Executor which handles session loss errors and alikes. By doing that, I got shocked by the sheer number of WTF issues I found. I'm sorry for ranting now, but it gets quicker to the poing. - Hostlist as string The hostlist is parsed in the ctor of ClientCnxn. This violates the rule of not doing (too much) work in a ctor. Instead the ClientCnxn should receive an object of class HostSet. HostSet could then be instantiated e.g. with a comma separated string. - cyclic dependency ClientCnxn, ZooKeeper ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. - Chroot is an attribute of ClientCnxn I'd like to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So I've components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. - deleteRecursive does not belong to the other methods DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? - massive code duplication in zookeeper class Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. - stat is returned by parameter Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. - stat calls static method on org.apache.zookeeper.server.DataTree It's a huge jump from client code to the internal server class DataTree. Shouldn't there rather be some class related to the protobuffer stat class that knows how to copy a stat? - Session class? Maybe it'd
Re: High WTF count in ZooKeeper client code
Hi Thomas, thanks for the reports, esp the JIRAs. One you missed (and aptly numbered) that's a pet peeve of mine is this one: https://issues.apache.org/jira/browse/ZOOKEEPER-666 I think part of what you are seeing is that the C api matured more quickly, and directly influenced the development of the java api. Given that the team is resource constrained we tend to prioritize fixes stability over code cleanup. Mahadev and I often talk about addressing these (and the server has some of this as well, esp given it's relative size) but more pressing issues keep us from getting to it. You seem pretty fired up to address these, I look forward to seeing some patches. Anyone can become a contributor, you'll find our how to contribute docs here: http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute Regards, Patrick On 08/11/2010 02:45 AM, Thomas Koch wrote: Hi, I started yesterday to work on my idea of an alternative ZooKeeper client interface.[1] Instead of methods on a ZooKeeper class, a user should instantiate an Operation (Create, Delete, ...) and forward it to an Executor which handles session loss errors and alikes. By doing that, I got shocked by the sheer number of WTF issues I found. I'm sorry for ranting now, but it gets quicker to the poing. - Hostlist as string The hostlist is parsed in the ctor of ClientCnxn. This violates the rule of not doing (too much) work in a ctor. Instead the ClientCnxn should receive an object of class HostSet. HostSet could then be instantiated e.g. with a comma separated string. - cyclic dependency ClientCnxn, ZooKeeper ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. - Chroot is an attribute of ClientCnxn I'd like to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So I've components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. - deleteRecursive does not belong to the other methods DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? - massive code duplication in zookeeper class Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. - stat is returned by parameter Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. - stat calls static method on org.apache.zookeeper.server.DataTree It's a huge jump from client code to the internal server class DataTree. Shouldn't there rather be some class related to the protobuffer stat class that knows how to copy a stat? - Session class? Maybe it'd make sense to combine hostlist, sessionId, sessionPassword and sessionTimeout in a Session class so that the ctor of ClientCnxn won't get too long? I may have missed some items. :-) Once again, please excuse my harsh tone. May I put the above issues in jira and would you accept (backwards compatible) patches for it for 3.4.0? Zookeeper is a fascinating project. Cudos to the devs. I've only looked in the client side code, which is what most users of zookeeper will ever see if they see any zookeeper internal code at all. So it may make sense to make this piece of the project as nice and clean as possible. [1] http://mail-archives.apache.org/mod_mbox/hadoop-zookeeper- dev/201005.mbox/%3c201005261509.54236.tho...@koch.ro%3e Best regards, Thomas Koch, http://www.koch.ro
[jira] Commented: (ZOOKEEPER-784) server-side functionality for read-only mode
[ https://issues.apache.org/jira/browse/ZOOKEEPER-784?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12897338#action_12897338 ] Henry Robinson commented on ZOOKEEPER-784: -- Spectacular job, Sergey. I've taken a look at the code and I'm pretty satisfied - you've done a great job covering little things like JMX support, and good code comments and documentation. I'm going to wait for one of the other committers to come by and also give this a +1 since this is a substantial change. We may also decide to run a long-lived test with this patch to satisfy ourselves of the stability. But this looks very, very solid indeed. server-side functionality for read-only mode Key: ZOOKEEPER-784 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-784 Project: Zookeeper Issue Type: Sub-task Components: server Reporter: Sergey Doroshenko Assignee: Sergey Doroshenko Fix For: 3.4.0 Attachments: ZOOKEEPER-784.patch, ZOOKEEPER-784.patch, ZOOKEEPER-784.patch, ZOOKEEPER-784.patch, ZOOKEEPER-784.patch, ZOOKEEPER-784.patch, ZOOKEEPER-784.patch, ZOOKEEPER-784.patch As per http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode , create ReadOnlyZooKeeperServer which comes into play when peer is partitioned. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-795) eventThread isn't shutdown after a connection session expired event coming
[ https://issues.apache.org/jira/browse/ZOOKEEPER-795?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12897366#action_12897366 ] Mahadev konar commented on ZOOKEEPER-795: - ben, pinging again, can you provide a patch for 3.3.2? eventThread isn't shutdown after a connection session expired event coming Key: ZOOKEEPER-795 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-795 Project: Zookeeper Issue Type: Bug Components: java client Affects Versions: 3.3.1 Environment: ubuntu 10.04 Reporter: mathieu barcikowski Assignee: Sergey Doroshenko Priority: Blocker Fix For: 3.3.2, 3.4.0 Attachments: ExpiredSessionThreadLeak.java, ZOOKEEPER-795.patch, ZOOKEEPER-795.patch Hi, I notice a problem with the eventThread located in ClientCnxn.java file. The eventThread isn't shutdown after a connection session expired event coming (i.e. never receive EventOfDeath). When a session timeout occurs and the session is marked as expired, the connexion is fully closed (socket, SendThread...) expect for the eventThread. As a result, if i create a new zookeeper object and connect through it, I got a zombi thread which will never be kill (as for the previous zookeeper object, the state is already close, calling close again don't do anything). So everytime I will create a new zookeeper connection after a expired session, I will have a one more zombi EventThread. How to reproduce : - Start a zookeeper client connection in debug mode - Pause the jvm enough time to the expired event occur - Watch for example with jvisualvm the list of threads, the sendThread is succesfully killed, but the EventThread go to wait state for a infinity of time - if you reopen a new zookeeper connection, and do again the previous steps, another EventThread will be present in infinite wait state -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-772) zkpython segfaults when watcher from async get children is invoked.
[ https://issues.apache.org/jira/browse/ZOOKEEPER-772?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Mahadev konar updated ZOOKEEPER-772: Status: Resolved (was: Patch Available) Resolution: Fixed Luckily the 3.4 patch applies to 3.3 branch. I just committed this. thanks henry! zkpython segfaults when watcher from async get children is invoked. --- Key: ZOOKEEPER-772 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-772 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Environment: ubuntu lucid (10.04) / zk trunk Reporter: Kapil Thangavelu Assignee: Henry Robinson Fix For: 3.3.2, 3.4.0 Attachments: asyncgetchildren.py, zkpython-testasyncgetchildren.diff, ZOOKEEPER-772.patch, ZOOKEEPER-772.patch When utilizing the zkpython async get children api with a watch, i consistently get segfaults when the watcher is invoked to process events. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-837) cyclic dependency ClientCnxn, ZooKeeper
[ https://issues.apache.org/jira/browse/ZOOKEEPER-837?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12897411#action_12897411 ] Thomas Koch commented on ZOOKEEPER-837: --- I've managed to make ClientCnxn independent from ZooKeeper. Could you give me please a first review of the changes from this branch? Thank you! http://github.com/thkoch2001/zookeeper/tree/ZOOKEEPER-837_uncycle_cxcn_from_zk cyclic dependency ClientCnxn, ZooKeeper --- Key: ZOOKEEPER-837 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-837 Project: Zookeeper Issue Type: Sub-task Affects Versions: 3.3.1 Reporter: Patrick Datko ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
Re: High WTF count in ZooKeeper client code
Off note: I have a Scala wrapper on the Java client (sync only) that has these functions: - treeAsList(path: String) - processTree(path: String,f: (String - Unit)) This implements recursive delete as a processTree(path,delete_), but also a recursive copy and move if need be. Just my two cents (when I'm back from holiday I will look at the contribute wiki and send add-ons) Best, Maarten On 08/11/2010 04:50 PM, Mahadev Konar wrote: Also, I am assuming you have backwards compatability in mind when you suggest these changes right? The interfaces of zookeeper client should not be changing as part of this, though the recursive delete hasn't been introduced yet (its only available in 3.4, so we can move it out into a helper class). Thanks mahadev On 8/11/10 7:40 AM, Mahadev Konarmaha...@yahoo-inc.com wrote: HI Thomas, I read through the list of issues you posted, most of them seem reasonable to fix. The one's you have mentioned below might take quite a bit of time to fix and again a lot of testing! (just a warning :)). It would be great if you'd want to clean this up for 3.4. Please go ahead and file a jira. These improvements would be good to have in the zookeeper java client. For deleteRecursive, I definitely agree that it should be a helper class. I don't believe it should be in the direct zookeeper api! Thanks mahadev On 8/11/10 2:45 AM, Thomas Kochtho...@koch.ro wrote: Hi, I started yesterday to work on my idea of an alternative ZooKeeper client interface.[1] Instead of methods on a ZooKeeper class, a user should instantiate an Operation (Create, Delete, ...) and forward it to an Executor which handles session loss errors and alikes. By doing that, I got shocked by the sheer number of WTF issues I found. I'm sorry for ranting now, but it gets quicker to the poing. - Hostlist as string The hostlist is parsed in the ctor of ClientCnxn. This violates the rule of not doing (too much) work in a ctor. Instead the ClientCnxn should receive an object of class HostSet. HostSet could then be instantiated e.g. with a comma separated string. - cyclic dependency ClientCnxn, ZooKeeper ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. - Chroot is an attribute of ClientCnxn I'd like to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So I've components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. - deleteRecursive does not belong to the other methods DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? - massive code duplication in zookeeper class Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. - stat is returned by parameter Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. - stat calls static method on org.apache.zookeeper.server.DataTree It's a huge jump from client code to the internal server class DataTree. Shouldn't there rather be some class related to the protobuffer stat class that knows how to copy a stat? - Session class? Maybe it'd make sense to combine hostlist, sessionId, sessionPassword and sessionTimeout in a Session class so that the ctor of ClientCnxn won't get too long? I may have missed some items. :-) Once again, please excuse my harsh tone. May I put the above issues in jira and would you accept (backwards compatible) patches for it for 3.4.0? Zookeeper is a fascinating project. Cudos to the devs. I've only looked in the client side code, which is what most users of zookeeper will
[jira] Updated: (ZOOKEEPER-837) cyclic dependency ClientCnxn, ZooKeeper
[ https://issues.apache.org/jira/browse/ZOOKEEPER-837?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Thomas Koch updated ZOOKEEPER-837: -- Attachment: ZOOKEEPER-837.patch cyclic dependency ClientCnxn, ZooKeeper --- Key: ZOOKEEPER-837 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-837 Project: Zookeeper Issue Type: Sub-task Affects Versions: 3.3.1 Reporter: Patrick Datko Assignee: Thomas Koch Attachments: ZOOKEEPER-837.patch ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-837) cyclic dependency ClientCnxn, ZooKeeper
[ https://issues.apache.org/jira/browse/ZOOKEEPER-837?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Thomas Koch updated ZOOKEEPER-837: -- Status: Patch Available (was: Open) first try without testing. Would like to hear your opinion about the direction. cyclic dependency ClientCnxn, ZooKeeper --- Key: ZOOKEEPER-837 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-837 Project: Zookeeper Issue Type: Sub-task Affects Versions: 3.3.1 Reporter: Patrick Datko Assignee: Thomas Koch Attachments: ZOOKEEPER-837.patch ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-839) deleteRecursive does not belong to the other methods
[ https://issues.apache.org/jira/browse/ZOOKEEPER-839?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Patrick Hunt updated ZOOKEEPER-839: --- Fix Version/s: 3.4.0 Priority: Blocker (was: Major) Component/s: java client blocker for 3.4, we don't want to ship this as a client api then change it later. deleteRecursive does not belong to the other methods Key: ZOOKEEPER-839 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-839 Project: Zookeeper Issue Type: Sub-task Components: java client Affects Versions: 3.3.1 Reporter: Patrick Datko Priority: Blocker Fix For: 3.4.0 DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (ZOOKEEPER-823) update ZooKeeper java client to optionally use Netty for connections
[ https://issues.apache.org/jira/browse/ZOOKEEPER-823?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Patrick Hunt updated ZOOKEEPER-823: --- Status: Patch Available (was: Open) update ZooKeeper java client to optionally use Netty for connections Key: ZOOKEEPER-823 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-823 Project: Zookeeper Issue Type: New Feature Components: java client Reporter: Patrick Hunt Assignee: Patrick Hunt Fix For: 3.4.0 Attachments: ZOOKEEPER-823.patch, ZOOKEEPER-823.patch This jira will port the client side connection code to use netty rather than direct nio. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
Re: High WTF count in ZooKeeper client code
Thomas, I see some patches already, which is great, however there's a big/complicated refactoring that's pending here: https://issues.apache.org/jira/browse/ZOOKEEPER-823 and to some extent here: https://issues.apache.org/jira/browse/ZOOKEEPER-733 and refactorings in this code prior to 733/823 going in are going to cause me much pain. (esp as I'm moving code around, creating new classes, etc) Could you hold off a bit on changes in this area until these two are committed? Ben is working on the reviews now. Ben please prioritize review/commit of these two. Patrick On 08/11/2010 08:23 AM, Thomas Koch wrote: Hallo Mahadev, thank you for your nice answer. Yes, we'll of cause preserve compatibility. Otherwise there is no chance to get accepted. I assume the following things must keep their interfaces: ZooKeeper (It'll call the new interface in the background), ASyncCallback, Watcher We may want to change: ClientCnxn (faktor out some things, remove dep on ZooKeeper) I think other classes should not be involved at all in our issues. My collegue Patrick was so kind to fill the jira issues. Best regards, Thomas Mahadev Konar: Also, I am assuming you have backwards compatability in mind when you suggest these changes right? The interfaces of zookeeper client should not be changing as part of this, though the recursive delete hasn't been introduced yet (its only available in 3.4, so we can move it out into a helper class). Thanks mahadev On 8/11/10 7:40 AM, Mahadev Konarmaha...@yahoo-inc.com wrote: HI Thomas, I read through the list of issues you posted, most of them seem reasonable to fix. The one's you have mentioned below might take quite a bit of time to fix and again a lot of testing! (just a warning :)). It would be great if you'd want to clean this up for 3.4. Please go ahead and file a jira. These improvements would be good to have in the zookeeper java client. For deleteRecursive, I definitely agree that it should be a helper class. I don't believe it should be in the direct zookeeper api! Thanks mahadev On 8/11/10 2:45 AM, Thomas Kochtho...@koch.ro wrote: Hi, I started yesterday to work on my idea of an alternative ZooKeeper client interface.[1] Instead of methods on a ZooKeeper class, a user should instantiate an Operation (Create, Delete, ...) and forward it to an Executor which handles session loss errors and alikes. By doing that, I got shocked by the sheer number of WTF issues I found. I'm sorry for ranting now, but it gets quicker to the poing. - Hostlist as string The hostlist is parsed in the ctor of ClientCnxn. This violates the rule of not doing (too much) work in a ctor. Instead the ClientCnxn should receive an object of class HostSet. HostSet could then be instantiated e.g. with a comma separated string. - cyclic dependency ClientCnxn, ZooKeeper ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. - Chroot is an attribute of ClientCnxn I'd like to have one process that uses ZooKeeper for different things (managing a list of work, locking some unrelated locks elsewhere). So I've components that do this work inside the same process. These components should get the same zookeeper-client reference chroot'ed for their needs. So it'd be much better, if the ClientCnxn would not care about the chroot. - deleteRecursive does not belong to the other methods DeleteRecursive has been committed to trunk already as a method to the zookeeper class. So in the API it has the same level as the atomic operations create, delete, getData, setData, etc. The user must get the false impression, that deleteRecursive is also an atomic operation. It would be better to have deleteRecursive in some helper class but not that deep in zookeeper's core code. Maybe I'd like to have another policy on how to react if deleteRecursive fails in the middle of its work? - massive code duplication in zookeeper class Each operation calls validatePath, handles the chroot, calls ClientCnxn and checks the return header for error. I'd like to address this with the operation classes: Each operation should receive a prechecked Path object. Calling ClientCnxn and error checking is not (or only partly) the concern of the operation but of an executor like class. - stat is returned by parameter Since one can return only one value in java it's the only choice to do so. Still it feels more like C then like Java. However with operator classes one could simply get the result values with getter functions after the execution. - stat calls static method on
[jira] Commented: (ZOOKEEPER-837) cyclic dependency ClientCnxn, ZooKeeper
[ https://issues.apache.org/jira/browse/ZOOKEEPER-837?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12897440#action_12897440 ] Patrick Hunt commented on ZOOKEEPER-837: Seems reasonable, notice that ZOOKEEPER-823 is pending and significantly changes this code. You might want to hold off until that gets committed. Nit - configure your editor to use spaces instead of tabs cyclic dependency ClientCnxn, ZooKeeper --- Key: ZOOKEEPER-837 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-837 Project: Zookeeper Issue Type: Sub-task Affects Versions: 3.3.1 Reporter: Patrick Datko Assignee: Thomas Koch Attachments: ZOOKEEPER-837.patch ZooKeeper instantiates ClientCnxn in its ctor with this and therefor builds a cyclic dependency graph between both objects. This means, you can't have the one without the other. So why did you bother do make them to separate classes in the first place? ClientCnxn accesses ZooKeeper.state. State should rather be a property of ClientCnxn. And ClientCnxn accesses zooKeeper.get???Watches() in its method primeConnection(). I've not yet checked, how this dependency should be resolved better. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-775) A large scale pub/sub system
[ https://issues.apache.org/jira/browse/ZOOKEEPER-775?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12897581#action_12897581 ] Benjamin Reed commented on ZOOKEEPER-775: - i believe the NOTICE file is consistent with: http://apache.org/legal/src-headers.html#header-existingcopyright A large scale pub/sub system Key: ZOOKEEPER-775 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-775 Project: Zookeeper Issue Type: New Feature Components: contrib Reporter: Benjamin Reed Assignee: Benjamin Reed Fix For: 3.4.0 Attachments: libs.zip, libs_2.zip, ZOOKEEPER-775.patch, ZOOKEEPER-775.patch, ZOOKEEPER-775.patch, ZOOKEEPER-775_2.patch, ZOOKEEPER-775_3.patch we have developed a large scale pub/sub system based on ZooKeeper and BookKeeper. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
