Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Zookeeper >> mail # dev >> ZooKeeper-442: watcher deregistration


Copy link to this message
-
Re: ZooKeeper-442: watcher deregistration
It would really be better if you put these comments into reviewboard, so
they will appear there and on the associated JIRA. The mailing list is a
bad place for code reviews.

Thanks!
Camille

On Tue, Jan 17, 2012 at 4:31 PM, Robert Crocombe <[EMAIL PROTECTED]> wrote:

> Well, I didn't get to look at it when I thought I would, and now I see
> there's a ReviewBoard thing.  I'd been working on some comments on and off:
> perhaps they would be valuable in the sense that they're from someone with
> little understanding of ZooKeeper's internals.  While working through the
> patch I also minimized it by removing whitespace-only changes and putting
> the imports back in their original order (one file dropped out of the patch
> as a result: it was only import order changes): I've attached that in case
> it might be of some use.  My comments are inline below.
>
> > diff --git a/src/java/main/org/apache/zookeeper/ClientCnxn.java
> b/src/java/main/org/apache/zookeeper/ClientCnxn.java
> > --- a/src/java/main/org/apache/zookeeper/ClientCnxn.java
> > +++ b/src/java/main/org/apache/zookeeper/ClientCnxn.java
> > @@ -53,6 +53,7 @@ import org.apache.zookeeper.Watcher.Even
> >  import org.apache.zookeeper.Watcher.Event.KeeperState;
> >  import org.apache.zookeeper.ZooDefs.OpCode;
> >  import org.apache.zookeeper.ZooKeeper.States;
> > +import org.apache.zookeeper.ZooKeeper.WatchDeregistration;
> >  import org.apache.zookeeper.ZooKeeper.WatchRegistration;
> >  import org.apache.zookeeper.client.HostProvider;
> >  import org.apache.zookeeper.client.ZooKeeperSaslClient;
> > @@ -203,6 +204,10 @@ public class ClientCnxn {
> >          return negotiatedSessionTimeout;
> >      }
> >
> > +    public Object getEventThreadLock() {
> > +        return eventThreadLock;
> > +    }
> > +
> >      @Override
> >      public String toString() {
> >          StringBuilder sb = new StringBuilder();
> > @@ -251,6 +256,8 @@ public class ClientCnxn {
> >
> >          WatchRegistration watchRegistration;
> >
>  > +        WatchDeregistration watchDeregistration;
> > +
> >          /** Convenience ctor */
> >          Packet(RequestHeader requestHeader, ReplyHeader replyHeader,
> >                 Record request, Record response,
> > @@ -380,6 +387,10 @@ public class ClientCnxn {
> >
> >      }
> >
> > +    public void queueCallback(AsyncCallback cb, int rc, String path,
> Object ctx) {
> > +        eventThread.queueCallback(cb, rc, path, ctx);
> > +    }
> > +
> >      // used by ZooKeeperSaslClient.queueSaslPacket().
> >      public void queuePacket(RequestHeader h, ReplyHeader r, Record
> request,
> >              Record response, AsyncCallback cb) {
> > @@ -424,6 +435,20 @@ public class ClientCnxn {
> >          }
> >      }
> >
> > +    private static class LocalCallback {
> > +        private final AsyncCallback cb;
> > +        private final int rc;
> > +        private final String path;
> > +        private final Object ctx;
> > +
>  > +        public LocalCallback(AsyncCallback cb, int rc, String path,
> Object ctx) {
> > +            this.cb = cb;
> > +            this.rc = rc;
> > +            this.path = path;
> > +            this.ctx = ctx;
> > +        }
> > +    }
> > +
> >      /**
> >       * Guard against creating
> "-EventThread-EventThread-EventThread-..." thread
> >       * names when ZooKeeper object is being created from within a
> watcher.
> > @@ -435,6 +460,8 @@ public class ClientCnxn {
> >          return name + suffix;
> >      }
>
> There should be documentation for what this lock synchronizes.  I believe
> the
> concern is that ZKWatchManager.materialize() and ZkWatchManager.remove()
> both
> access the members dataWatches, existWatches, and childWatches.  However,
> within both materialize() and remove() there are individual synchronized
> blocks protecting the maps individually.  Is the concern therefore that you
> need a lock across all of them simultaneously, or have I missed something?
>  If
> not, mightn't it better to simply make materialize() and remove() both