Re: High WTF count in ZooKeeper client code

2010-09-07 Thread Patrick Hunt
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 maha...@yahoo-inc.comwrote:

 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 maha...@yahoo-inc.com 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 ph...@apache.org 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 Konarmaha...@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 Kochtho...@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, 

Re: High WTF count in ZooKeeper client code

2010-09-04 Thread Mahadev Konar
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 maha...@yahoo-inc.com 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 ph...@apache.org 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 Konarmaha...@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 Kochtho...@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 

Re: High WTF count in ZooKeeper client code

2010-08-31 Thread Mahadev Konar
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 ph...@apache.org 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 Konarmaha...@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 Kochtho...@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 

Re: High WTF count in ZooKeeper client code

2010-08-12 Thread Thomas Koch
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 Konarmaha...@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 Kochtho...@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 

Re: High WTF count in ZooKeeper client code

2010-08-12 Thread Patrick Hunt


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 Konarmaha...@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 Kochtho...@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 

Re: High WTF count in ZooKeeper client code

2010-08-12 Thread Patrick Hunt
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 Konarmaha...@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 Kochtho...@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 

Re: High WTF count in ZooKeeper client code

2010-08-11 Thread Mahadev Konar
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



Re: High WTF count in ZooKeeper client code

2010-08-11 Thread 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




Re: High WTF count in ZooKeeper client code

2010-08-11 Thread Thomas Koch
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 

Re: High WTF count in ZooKeeper client code

2010-08-11 Thread Patrick Hunt
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 Konarmaha...@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 Kochtho...@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 

Re: High WTF count in ZooKeeper client code

2010-08-11 Thread Patrick Hunt
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

2010-08-11 Thread Maarten Koopmans
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 Konarmaha...@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 Kochtho...@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 

Re: High WTF count in ZooKeeper client code

2010-08-11 Thread 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 Konarmaha...@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 Kochtho...@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