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 (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
