----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review64767 -----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107547> Can you give thread name for this thread pool like, Executors.newCachedThreadPool(new ThreadFactoryBuilder() .setNameFormat("ZooKeeper-NIOBoss-%d").build()), Executors.newCachedThreadPool(new ThreadFactoryBuilder() .setNameFormat("ZooKeeper-NIOWorker-%d").build()) src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107544> Can we make the visibiliy of the member variables to private. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107543> Could you tell me when this situation can come? Is this added for safer side? src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107526> bootstrap.setOption("connectTimeoutMillis", System.getProperty("connectTimeoutMillis")); What do you think about providing 'clientConnectTimeoutMillis' as a configurable item. By default its 10secs. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107527> Before return add log msg, it will help in debugging/analysing failure cases. Also, when loggin get the cause from the future object. future.getCause() src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107531> During logging it can log the channel details like channelFuture.getChannel(), consider multiple zkclients running in same JVM. The same comment is applicable wherever doing channel related logs. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107533> Make it private static class WakeupPacket src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107528> Add channel details in the log like, LOG.info("channel {} is disconnected", ctx.getChannel()); src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107535> use {} instead of + - Rakesh R 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 > >
