> On Nov. 1, 2014, 12:14 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java, line 154 > > <https://reviews.apache.org/r/27244/diff/5/?file=745331#file745331line154> > > > > "... already started sending...". Also, the comment is overflowing, so > > moving it to the top sounds like a good choice.
This is old code. I only make this a common static func. I will fix it. > On Nov. 1, 2014, 12:14 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66 > > <https://reviews.apache.org/r/27244/diff/6/?file=746460#file746460line66> > > > > Shouldn't isConnected return true in the case channel isn't null > > instead of channelFactory? In fact, it sounds like this implementation is > > instantiating a new ChannelFactory every time it tries to connect (modulo > > an attempt already being in progress). It doesn't sound necessary. I thought the same thing. Here I am writing as similar code as I could to ZOOKEEPER-723 (Patrick's previous patch). I will change to use `channel` instead. Thanks! > On Nov. 1, 2014, 12:14 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 88 > > <https://reviews.apache.org/r/27244/diff/6/?file=746460#file746460line88> > > > > Could you use the future object it returns as opposed to using the > > firstConnect CountDownLatch? I think you're using the latch to determine if > > the connection request is still pending. It needs to do some initialization work before entering doTransportation. I am wondering if that `Future` object from connect could ensure that. I have asked a question to netty dev and waiting for response. > On Nov. 1, 2014, 12:14 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 165 > > <https://reviews.apache.org/r/27244/diff/6/?file=746460#file746460line165> > > > > A message like "outgoingQueue isn't empty, but we haven't been > > notified". Also, I'm not sure how this could happen, is this a potential > > race or just a sanity check? It's something I am not sure about. I am worried that I might dismiss some places to wake it up. As the comment stated, *last straw*. - Hongchao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review59363 ----------------------------------------------------------- On Oct. 31, 2014, 10:57 p.m., Hongchao Deng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2014, 10:57 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > ------- > > ZOOKEEPER-2069 > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeper.java dd13cc9 > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > ------- > > > Thanks, > > Hongchao Deng > >
