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, 
We may want to change: ClientCnxn (faktor out some things, remove dep on 

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,


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

Reply via email to