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

 File path: 
 @@ -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:

With regards,
Apache Git Services

Reply via email to