[GitHub] [zookeeper] eribeiro commented on a change in pull request #1108: ZOOKEEPER-2238: Support limiting the maximum number of connections/clients to a zookeeper server

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

eribeiro commented on a change in pull request #1108: ZOOKEEPER-2238: Support limiting the maximum number of connections/clients to a zookeeper server
URL: https://github.com/apache/zookeeper/pull/1108#discussion_r343302097
 
 

 ##########
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/NIOServerCnxnFactory.java
 ##########
 @@ -273,6 +273,10 @@ private boolean doAccept() {
             try {
                 sc = acceptSocket.accept();
                 accepted = true;
+                if (limitTotalNumberOfCnxns()) {
+                    accepted = false;
+                    return accepted;
+                }
 
 Review comment:
   Hey, thanks for picking up this issue! :smile: I couldn't finish it years ago due to other commitments (shame on me). Your PR looks really nice and better, congratulations. :+1: 
   
   I have only one question: are you sure that by returning from the method here are you not leaking resources by not closing `sc` channel? 
   
   See lines 283-285 where we throw an exception if a specific client exceeds its max connections. This exception is then catch at line 301. There we increment the `ServerMetrics.getMetrics().CONNECTION_REJECTED` counter and close the channel at `fastCloseSock(sc);`.
   
   I would recommend to do something similar (throw an exception), and removing the increment of the counter from `limitTotalNumberOfCnxns` (IMO, it's a query method, but incrementing the counter there we can lead to miscalculations if any other code uses it elsewhere). This would require to add the counter in `NettyServerCnxnFactory` too. Wdyt?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services