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.com>wrote: > 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 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 > > > > > >