> On Dec. 2, 2014, 11:43 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66 > > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line66> > > > > Is the reason to have workSemaphore to block in doTransport when there > > isn't anything in the outgoing queue? If so, we might be better off with a > > blocking queue rather than a list as we have right now. > > Hongchao Deng wrote: > Sounds good on my side. I tried to make minimal changes. Do you think I > should make it in here or create another JIRA?
I'd rather have it fixed here, but depends on how much more energy you have to put into this patch. > On Dec. 2, 2014, 11:43 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 107 > > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line107> > > > > I'm not sure why you're not using the ChannelFuture returned here. See > > this connect() method in bookkeeper for an example > > https://github.com/apache/bookkeeper/blob/branch-4.0/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L112 > > Hongchao Deng wrote: > Good. Fix it next Ok. > On Dec. 2, 2014, 11:43 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188 > > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188> > > > > Could you elaborate on what you're trying to do with this NIOLock? I'm > > not sure what you mean with race cocnditions in the comment. > > Hongchao Deng wrote: > 1. The intuition is that in NIO client all reads and writes are > sequential. > 2. One problem I found: (1) packet is removed from outgoingQueue; (2) > before the packet is added to pendingQueue; (3) sendThread.readResponse > throws exception. > 3. I am wondering what the purpose of pendingQueue is except for checking > and if we can remove it. AFAICT that's the only concurrency concern. > > Hongchao Deng wrote: > I come up with a better way if the pendingQueue is the only concern. It > looks like pendingQueue doesn't care about ordering. So I can add packets to > pendingQueue before writing to channel. Check SendThread.readResponse() in ClientCnxn, pls. - fpj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review63604 ----------------------------------------------------------- On Dec. 3, 2014, 1:52 a.m., Hongchao Deng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2014, 1:52 a.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/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 > ------- > > 1. use ChannelFuture returned by connect() > 2. remove NIOLock > > > Thanks, > > Hongchao Deng > >
