> On Dec. 11, 2014, 10:53 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 70 > > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line70> > > > > I see, the previous sasl changes are necessary here so that we can > > proceed only if authentication suceeds. Please confirm. > > Hongchao Deng wrote: > I change enableWrite(), enableReadWriteOnly(), WakeupCnxn() interfaces: > 1. enable read, write is renaming only. Because in netty it doesn't make > sense to say enable write. It's more natural to say SASL completed and what > we are doing here. > 2. I split WakeupCnxn() into two because they are for two purposes: one > is for notifying packet has been added to outgoingQueue, the other is for > notifying connection is intentionally closed. They are different in Netty.
Ok, got it. Netty 3 actually allowed you to control those flags directly, but apparently they got rid of it in Netty 4. See this http://stackoverflow.com/questions/24888912/where-is-channel-setinterestops-in-netty-4x I think you're saying that the enableWrite call was there only for sasl so you prefer to just name explicitly? If that's the intention, then seems good to me. > On Dec. 11, 2014, 10:53 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 68 > > <https://reviews.apache.org/r/27244/diff/28/?file=789549#file789549line68> > > > > Why does pendingQueue need to be made concurrent? > > Hongchao Deng wrote: > I will convert it back. If it is worth having this optmization, then I suggest you propose it in another jira. You have the changes already, it should be easy to generate a patch. > On Dec. 11, 2014, 10:53 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1372 > > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line1372> > > > > If these SASL changes aren't strictly necessary for this patch, we > > should do them in a different jira. > > Hongchao Deng wrote: > This is commented by Raul @rgs to change.. I usually didn't change name > myself. My question wasn't about the name change, but instead about the changes around sasl. I got what you're trying to do now. - fpj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review64803 ----------------------------------------------------------- On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > ----------------------------------------------------------- > > (Updated Dec. 11, 2014, 6:11 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > ------- > > ZOOKEEPER-2069 > > > Diffs > ----- > > build.xml bb5ff4f > 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/ZooKeeperTestable.java 775d1a2 > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080 > src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c > 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 LinkedBlockingDeque. > > > Thanks, > > Hongchao Deng > >
