[
https://issues.apache.org/jira/browse/ZOOKEEPER-2280?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14909960#comment-14909960
]
Raul Gutierrez Segales commented on ZOOKEEPER-2280:
---------------------------------------------------
Thanks for the patch [~eribeiro]. A few nits:
In:
{code}
+ LOG.error("Too many connections from " + addr + " - max is " +
maxClientCnxns);
{code}
can we please us interpolation ({}) instead of concatenation (+)?
After that line, in:
{code}
+ cnxn.close();
+ ctx.getChannel().close().awaitUninterruptibly();
{code}
Can any of that throw anything? If so, should we - silently - catch that and
ignore?
In getClientCnxnCount:
{code}
+ if (s == null) return 0;
+ return s.size();
{code}
you can do it in one line (or use braces if you want multiple lines):
{code}
return s == null ? 0 : s.size();
{code}
In testMaxClientConnections() a small nit:
{code}
+ @Test(timeout = 40000)
+ public void testMaxClientConnections() throws Exception {
{code}
with indentation.
In that file as well, lets try not to sleep:
{code}
+ TestableZooKeeper[] clients = new TestableZooKeeper[CLIENTS];
+ for (int i = 0; i < CLIENTS; i++) {
+ clients[i] = new TestableZooKeeper("127.0.0.1:" + CLIENT_PORT,
3000, new CountdownWatcher());
+ }
+
+ Thread.sleep(5000);
+
+ assertEquals(MAX_CLIENT_CONN, scf.getNumAliveConnections());
{code}
I guess you are sleeping to wait for the clients[] to be connnected? Calling
waitForConnected for each would a worse, so how about implementing a Watcher
that counts the KeeperState.SyncConnected events seen. And then you can wait
for that watcher to reach CLIENTS.
Super nit, in:
{code}
+ }
+ finally {
+ scf.shutdown();
+ zks.shutdown();
+ }
{code}
closing curly brace and finally in the same line pls (and ditto in
testConnections()), i.e.:
{code}
} finally {
scf.shutdown();
zks.shutdown();
}
{code}
Finally, testConnections() and testMaxConnections() are pretty much the same
except one asserts MAX_CLIENT_CONN == connected and the other one asserts
CLIENTS == connected. Can we put the body of these tests in a helper (i.e.:
testConns) and just call it with the limit we want to check?
Thanks!
> NettyServerCnxnFactory doesn't honor maxClientCnxns param
> ---------------------------------------------------------
>
> Key: ZOOKEEPER-2280
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2280
> Project: ZooKeeper
> Issue Type: Bug
> Components: server
> Affects Versions: 3.4.6, 3.5.0, 3.5.1
> Reporter: Edward Ribeiro
> Assignee: Edward Ribeiro
> Fix For: 3.5.1
>
> Attachments: ZOOKEEPER-2280.patch
>
>
> Even though NettyServerCnxnFactory has maxClientCnxns (default to 60) it
> doesn't enforce this limit in the code.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
