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 

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-

Best regards,

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

Reply via email to