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 Hunt:

I see some patches already, which is great, however there's a
big/complicated refactoring that's pending here:


and to some extent here:

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.


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

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).


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!


On 8/11/10 2:45 AM, "Thomas Koch"<tho...@koch.ro>   wrote:


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

- 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

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-

Best regards,

Thomas Koch, http://www.koch.ro

Thomas Koch, http://www.koch.ro

Reply via email to