Re: High WTF count in ZooKeeper client code
However it should be pretty easy to apply though given our stability and relatively small size. :-) Seems like it would be useful for users and developers both... Patrick On Fri, Sep 3, 2010 at 11:49 PM, Mahadev Konar wrote: > I was able to get hold of one of the hadoop developers. So the gist of the > story is, > > They have interface tagging saying > > Something like > > @Audience.limitedPrivate(target="pig") > > Wherin this interface is defined for pig and is only to be used by pig > oflks. > > Interfaces can be defined as public, stable, unstable, .. > > This is quite useful but given out interfaces havent chanhged in a long > time > this might not be that helpful for us. > > Thanks > mahadev > > > On 8/31/10 3:47 PM, "Mahadev Konar" wrote: > > > There isnt any documentation on the interface tagging other than the > running > > comments. I will try to get hold of one of the hadoop folks to get me a > dump > > of the info and will create a jira! > > > > Thanks > > mahadev > > > > > > On 8/11/10 9:56 AM, "Patrick Hunt" wrote: > > > > 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 Konar" 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" 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 the
Re: High WTF count in ZooKeeper client code
I was able to get hold of one of the hadoop developers. So the gist of the story is, They have interface tagging saying Something like @Audience.limitedPrivate(target="pig") Wherin this interface is defined for pig and is only to be used by pig oflks. Interfaces can be defined as public, stable, unstable, .. This is quite useful but given out interfaces havent chanhged in a long time this might not be that helpful for us. Thanks mahadev On 8/31/10 3:47 PM, "Mahadev Konar" wrote: > There isnt any documentation on the interface tagging other than the running > comments. I will try to get hold of one of the hadoop folks to get me a dump > of the info and will create a jira! > > Thanks > mahadev > > > On 8/11/10 9:56 AM, "Patrick Hunt" wrote: > > 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 Konar" 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" 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 d
Re: High WTF count in ZooKeeper client code
There isnt any documentation on the interface tagging other than the running comments. I will try to get hold of one of the hadoop folks to get me a dump of the info and will create a jira! Thanks mahadev On 8/11/10 9:56 AM, "Patrick Hunt" wrote: 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 Konar" 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" 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
Re: High WTF count in ZooKeeper client code
Thomas, btw, if you'd like (anyone really) to do a patch extracting deleterecursive from zk into some helper class I think that would be a good idea to get sooner rather than later. Patrick On 08/11/2010 11:36 PM, Thomas Koch wrote: Patrick, I saw your patch and was afraid you wouldn't like to wait for me and change it. :-) I'll continue to work on my issues and also put them into jira for review so that my team can start to work on the new API. After your patch is applied, I'll adapt my patches, which should not change anything to the user facing API of ZK. Thomas Patrick Hunt: 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 Konar" 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" 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 retu
Re: High WTF count in ZooKeeper client code
On 08/11/2010 11:36 PM, Thomas Koch wrote: I saw your patch and was afraid you wouldn't like to wait for me and change it. :-) I'll continue to work on my issues and also put them into jira for review so that my team can start to work on the new API. After your patch is applied, I'll adapt my patches, which should not change anything to the user facing API of ZK. No problem, just didn't want you to get frustrated in case you didn't notice. As long as you go in with eyes wide open it's ok w/me. :-) Patrick Patrick Hunt: 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 Konar" 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" 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 oper
Re: High WTF count in ZooKeeper client code
Patrick, I saw your patch and was afraid you wouldn't like to wait for me and change it. :-) I'll continue to work on my issues and also put them into jira for review so that my team can start to work on the new API. After your patch is applied, I'll adapt my patches, which should not change anything to the user facing API of ZK. Thomas Patrick Hunt: > 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 Konar" 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" 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 zookeep
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 Konar" 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" 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
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 Konar" 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" 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 zoo
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
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 Konar" 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" 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 hos
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" 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" 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 excus
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" 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" 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
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" 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