Home | About | Sematext search-lucene.com search-hadoop.com
NEW: Monitor These Apps!
elasticsearch, apache solr, apache hbase, hadoop, redis, casssandra, amazon cloudwatch, mysql, memcached, apache kafka, apache zookeeper, apache storm, ubuntu, centOS, red hat, debian, puppet labs, java, senseiDB
 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
NEW: Monitor These Apps!
elasticsearch, apache solr, apache hbase, hadoop, redis, casssandra, amazon cloudwatch, mysql, memcached, apache kafka, apache zookeeper, apache storm, ubuntu, centOS, red hat, debian, puppet labs, java, senseiDB