[GitHub] [zookeeper] eolivelli commented on a change in pull request #1770: Netty server does not respond to ruok while initializing cluster

[GitBox <gi...@apache.org>]

eolivelli commented on a change in pull request #1770:
URL: https://github.com/apache/zookeeper/pull/1770#discussion_r785712241



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java
##########
@@ -225,15 +225,6 @@ public void channelActive(ChannelHandlerContext ctx) throws Exception {
             NettyServerCnxn cnxn = new NettyServerCnxn(channel, zkServer, NettyServerCnxnFactory.this);
             ctx.channel().attr(CONNECTION_ATTRIBUTE).set(cnxn);
 
-            // Check the zkServer assigned to the cnxn is still running,
-            // close it before starting the heavy TLS handshake
-            if (!cnxn.isZKServerRunning()) {
-                LOG.warn("Zookeeper server is not running, close the connection before starting the TLS handshake");
-                ServerMetrics.getMetrics().CNXN_CLOSED_WITHOUT_ZK_SERVER_RUNNING.add(1);

Review comment:
       I am re-reading the patch again.
   It looks like we are no more updating this metric.
   and also we are dropping this case.
   
   My understanding is that this check is here to prevent a flood of useless (but heavyweight) TLS handshakes after restarting the ZK node.
   
   I am not sure this is a good move to remove this. 
   This fix may work on a small cluster (with very few ZK clients I mean)
   
   @lvfangmin If I read correctly (from git blame) this improvement was part of ZOOKEEPER-3682 and the set of patches ported from Facebook ZooKeeper fork.
   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org