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
synchronized, or does that introduce some bottleneck (I don't see it)?

I suppose this change introduces the minimum amount of synchronization
necessary (least change vs. existing behavior), but I guess I'd prefer to
see
it fixed for any potential uses cases now rather than have the next person
discover belatedly that he/she needs to have this kind of locking.

> +    private final Object eventThreadLock = new Object();
Object ctx) {
event.getType(),
event.getType(),
processing
lcb.path, lcb.ctx, null);
lcb.path, lcb.ctx, null, null);
lcb.path, lcb.ctx, null, null);
lcb.path, lcb.ctx, null);
lcb.path, lcb.ctx, null, null);
lcb.path, lcb.ctx, null);
lcb.path, lcb.ctx);

Ugh.  I suppose there's no help for it, though.

null);
WatchDeregistration watchDeregistration)
null,
watchRegistration)
serverPath, ctx, watchRegistration, null);
watchRegistration,
b/src/java/main/org/apache/zookeeper/KeeperException.java
code");

"nonexistent"?

b/src/java/main/org/apache/zookeeper/Watcher.java
b/src/java/main/org/apache/zookeeper/ZooDefs.java
b/src/java/main/org/apache/zookeeper/ZooKeeper.java
watches, Watcher watcher, String path) {
Watcher watcher, String path, boolean local) {

Please elaborate on why 'local' is necessary: I see that remove() below is
called with 'true' in ZooKeeper.removeWatches() and 'false' in
WatcherDeregistration.unregister(), but I decided I was too lazy to parse
the
semantics of the behavior difference in the two cases and why they are
necessary without any documentation.

watcher set on the znode
ben triggered by a server's message
WatcherType type, boolean local) throws KeeperException {
clientPath);
clientPath, local);
clientPath);
clientPath, local)) {
watcher, clientPath, local)) {
String clientPath)

Looks like copy/paste error from WatchRegistration.  The "add" should be
"remove".

the
triggered.
serverside.
be thrown
interrupted.
non-zero
type)

So it looks like this loop is here to suppress InterruptedExceptions and
keep
trying the exchange() until it works.  I believe this was done so that the
opposing thread in the exchange will not be left blocked in exchange()
forever (as you noted in the comment below).  I thought I'd found a case
where
the loop was infinite if an exception was thrown before queueCallback()
could
be called, but it looks like you have that covered, too.  Uhm, seems like
it'd
be good to document this since I spent about 10 minutes noodlin

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