You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by eribeiro <gi...@git.apache.org> on 2017/03/25 04:14:17 UTC
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
GitHub user eribeiro opened a pull request:
https://github.com/apache/zookeeper/pull/208
ZOOKEEPER-2280: NettyServerCnxnFactory doesn't implement maxClientCnxns
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eribeiro/zookeeper ZOOKEEPER-2280
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/208.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #208
----
commit ceaa5a2f0cf851620a6e61a84e597d30d42da131
Author: Edward Ribeiro <ed...@gmail.com>
Date: 2017-03-25T03:59:12Z
ZOOKEEPER-2280: NettyServerCnxnFactory doesn't implement maxClientCnxns
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108256234
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -524,17 +538,14 @@ public InetSocketAddress getLocalAddress() {
private void addCnxn(NettyServerCnxn cnxn) {
cnxns.add(cnxn);
- synchronized (ipMap){
- InetAddress addr =
- ((InetSocketAddress)cnxn.channel.getRemoteAddress())
- .getAddress();
- Set<NettyServerCnxn> s = ipMap.get(addr);
- if (s == null) {
- s = new HashSet<NettyServerCnxn>();
- }
- s.add(cnxn);
- ipMap.put(addr,s);
+
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
--- End diff --
Indeed, there's too much call for casting in this class already. Are you suggesting to extract this line into a method?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108256938
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -524,17 +538,14 @@ public InetSocketAddress getLocalAddress() {
private void addCnxn(NettyServerCnxn cnxn) {
cnxns.add(cnxn);
- synchronized (ipMap){
- InetAddress addr =
- ((InetSocketAddress)cnxn.channel.getRemoteAddress())
- .getAddress();
- Set<NettyServerCnxn> s = ipMap.get(addr);
- if (s == null) {
- s = new HashSet<NettyServerCnxn>();
- }
- s.add(cnxn);
- ipMap.put(addr,s);
+
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
--- End diff --
we can pass the InetAddress as a param?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108027783
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -109,6 +110,20 @@ public void channelConnected(ChannelHandlerContext ctx,
zkServer, NettyServerCnxnFactory.this);
ctx.setAttachment(cnxn);
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = ipMap.get(addr);
+ if (s != null) {
+ synchronized (s) {
+ int cnxncount = s.size();
+ if (maxClientCnxns > 0 && cnxncount >= maxClientCnxns) {
+ LOG.warn("Closed new connection from address {} ({} connections established) - max is {}",
+ addr, cnxncount, maxClientCnxns);
+ cnxn.close();
+ ctx.getChannel().close().awaitUninterruptibly();
--- End diff --
TODO: double-check that this is the right order of closing cnxn and channel
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108242052
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -100,11 +101,15 @@ public void close() {
+ Long.toHexString(sessionId));
}
- synchronized (factory.ipMap) {
- Set<NettyServerCnxn> s =
- factory.ipMap.get(((InetSocketAddress)channel
- .getRemoteAddress()).getAddress());
- s.remove(this);
+ InetAddress address = ((InetSocketAddress) channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = factory.ipMap.get(address);
+ if (s != null) {
+ synchronized (s) {
--- End diff --
Yeah... in the context of this call I think it is not necessary to synchronize here if -- as you said -- we use a synchronized set (**currently it's not**). I would also change `ipMap` to be a `ConcurrentMap` but unsure if it's worth the effort, tbh. :thinking: thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108220399
--- Diff: src/java/test/org/apache/zookeeper/server/NettyServerCnxnTest.java ---
@@ -28,6 +32,16 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.File;
+import java.net.InetAddress;
+import java.util.Set;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.CountDownLatch;
+
+import static junit.framework.TestCase.assertEquals;
+import static junit.framework.TestCase.assertNotNull;
--- End diff --
i don't believe this import is being used
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108229544
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -100,11 +101,15 @@ public void close() {
+ Long.toHexString(sessionId));
}
- synchronized (factory.ipMap) {
- Set<NettyServerCnxn> s =
- factory.ipMap.get(((InetSocketAddress)channel
- .getRemoteAddress()).getAddress());
- s.remove(this);
+ InetAddress address = ((InetSocketAddress) channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = factory.ipMap.get(address);
--- End diff --
I agree that that ipMap management would be cleaner if it was handled in the factory.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108242965
--- Diff: src/java/test/org/apache/zookeeper/server/NettyServerCnxnTest.java ---
@@ -84,4 +98,78 @@ public void testSendCloseSession() throws Exception {
zk.close();
}
}
+
+ @Test(timeout = 30000)
+ public void testMaxClientConnectionsReached() throws Exception {
+ final int maxClientCnxns = 4;
+ final int numClients = 10;
+ createAndTestConnections(numClients, maxClientCnxns, maxClientCnxns);
+ }
+
+ @Test(timeout = 30000)
+ public void testMaxClientConnectionsDisabled() throws Exception {
+ final int maxClientCnxns = 0; // disabled cnxns limit
+ final int numClients = 10;
+ createAndTestConnections(numClients, maxClientCnxns, numClients);
+ }
+
+ private void createAndTestConnections(int numClients, int maxClientCnxns, int cnxnsAccepted) throws Exception {
+
+ File tmpDir = ClientBase.createTmpDir();
+ final int CLIENT_PORT = PortAssignment.unique();
+
+ ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
+ ServerCnxnFactory scf = ServerCnxnFactory.createFactory(CLIENT_PORT, maxClientCnxns);
+ scf.startup(zks);
+
+ try {
+ assertTrue("waiting for server being up",
+ ClientBase.waitForServerUp("127.0.0.1:" + CLIENT_PORT, CONNECTION_TIMEOUT));
+ assertTrue("Didn't instantiate ServerCnxnFactory with NettyServerCnxnFactory!",
+ scf instanceof NettyServerCnxnFactory);
+
+ assertEquals(0, scf.getNumAliveConnections());
+
+ assertTrue(cnxnsAccepted <= numClients);
+
+ final CountDownLatch countDownLatch = new CountDownLatch(cnxnsAccepted);
+
+ TestableZooKeeper[] clients = new TestableZooKeeper[numClients];
+ for (int i = 0; i < numClients; i++) {
+ clients[i] = new TestableZooKeeper("127.0.0.1:" + CLIENT_PORT, 3000, new Watcher() {
+ @Override
+ public void process(WatchedEvent event)
+ {
+ if (event.getState() == Event.KeeperState.SyncConnected) {
+ countDownLatch.countDown();
+ }
+ }
+ });
+ }
+
+ countDownLatch.await();
+
+ assertEquals(cnxnsAccepted, scf.getNumAliveConnections());
+
+ ConcurrentMap<InetAddress, Set<NettyServerCnxn>> ipMap = ((NettyServerCnxnFactory) scf).ipMap;
+ assertEquals(1, ipMap.size());
+ Set<NettyServerCnxn> set = ipMap.get(ipMap.keySet().toArray()[0]);
+ assertEquals(cnxnsAccepted, set.size());
+
+ int connected = 0;
+ for (int i = 0; i < numClients; i++) {
+ if (clients[i].getState().isConnected()) connected++;
--- End diff --
Oops, indeed! +1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108242887
--- Diff: src/java/test/org/apache/zookeeper/server/NettyServerCnxnTest.java ---
@@ -84,4 +98,78 @@ public void testSendCloseSession() throws Exception {
zk.close();
}
}
+
+ @Test(timeout = 30000)
+ public void testMaxClientConnectionsReached() throws Exception {
+ final int maxClientCnxns = 4;
+ final int numClients = 10;
+ createAndTestConnections(numClients, maxClientCnxns, maxClientCnxns);
+ }
+
+ @Test(timeout = 30000)
+ public void testMaxClientConnectionsDisabled() throws Exception {
+ final int maxClientCnxns = 0; // disabled cnxns limit
+ final int numClients = 10;
+ createAndTestConnections(numClients, maxClientCnxns, numClients);
+ }
+
+ private void createAndTestConnections(int numClients, int maxClientCnxns, int cnxnsAccepted) throws Exception {
+
+ File tmpDir = ClientBase.createTmpDir();
+ final int CLIENT_PORT = PortAssignment.unique();
+
+ ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
--- End diff --
+1. :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108239884
--- Diff: src/java/test/org/apache/zookeeper/server/NettyServerCnxnTest.java ---
@@ -51,9 +65,9 @@ public void setUp() throws Exception {
*
* @see <a href="https://issues.jboss.org/browse/NETTY-412">NETTY-412</a>
*/
- @Test(timeout = 40000)
+ @Test(timeout = 30000)
--- End diff --
Due to @fpj previous review on previous incarnation this patch: https://github.com/apache/zookeeper/pull/77/files#r80003749 (sorry, should have put the original link).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108219703
--- Diff: src/java/test/org/apache/zookeeper/server/NettyServerCnxnTest.java ---
@@ -51,9 +65,9 @@ public void setUp() throws Exception {
*
* @see <a href="https://issues.jboss.org/browse/NETTY-412">NETTY-412</a>
*/
- @Test(timeout = 40000)
+ @Test(timeout = 30000)
--- End diff --
why was this changed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108231907
--- Diff: src/java/test/org/apache/zookeeper/server/NettyServerCnxnTest.java ---
@@ -84,4 +98,78 @@ public void testSendCloseSession() throws Exception {
zk.close();
}
}
+
+ @Test(timeout = 30000)
+ public void testMaxClientConnectionsReached() throws Exception {
+ final int maxClientCnxns = 4;
+ final int numClients = 10;
+ createAndTestConnections(numClients, maxClientCnxns, maxClientCnxns);
+ }
+
+ @Test(timeout = 30000)
+ public void testMaxClientConnectionsDisabled() throws Exception {
+ final int maxClientCnxns = 0; // disabled cnxns limit
+ final int numClients = 10;
+ createAndTestConnections(numClients, maxClientCnxns, numClients);
+ }
+
+ private void createAndTestConnections(int numClients, int maxClientCnxns, int cnxnsAccepted) throws Exception {
+
+ File tmpDir = ClientBase.createTmpDir();
+ final int CLIENT_PORT = PortAssignment.unique();
+
+ ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
--- End diff --
it seems strange to me to have some tests that start their on zookeeper server and another which uses the one provided by the @before from the superclass. I think we should pick one approach per test class. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108240078
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -170,7 +185,6 @@ public void messageReceived(ChannelHandlerContext ctx, MessageEvent e)
throw ex;
}
}
-
--- End diff --
Ops, again Flavio's previous review comment: https://github.com/apache/zookeeper/pull/77/files#r80004333 (should have put referenced this here).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108028293
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -109,6 +110,20 @@ public void channelConnected(ChannelHandlerContext ctx,
zkServer, NettyServerCnxnFactory.this);
ctx.setAttachment(cnxn);
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = ipMap.get(addr);
+ if (s != null) {
+ synchronized (s) {
--- End diff --
Same here - check if synchronized is needed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108027802
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -100,11 +101,15 @@ public void close() {
+ Long.toHexString(sessionId));
}
- synchronized (factory.ipMap) {
- Set<NettyServerCnxn> s =
- factory.ipMap.get(((InetSocketAddress)channel
- .getRemoteAddress()).getAddress());
- s.remove(this);
+ InetAddress address = ((InetSocketAddress) channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = factory.ipMap.get(address);
--- End diff --
TODO: "This management of the ipMap looks like something that should be done by the factory, not the connection. should we move this logic to the factory?"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108028137
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -100,11 +101,15 @@ public void close() {
+ Long.toHexString(sessionId));
}
- synchronized (factory.ipMap) {
- Set<NettyServerCnxn> s =
- factory.ipMap.get(((InetSocketAddress)channel
- .getRemoteAddress()).getAddress());
- s.remove(this);
+ InetAddress address = ((InetSocketAddress) channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = factory.ipMap.get(address);
+ if (s != null) {
+ synchronized (s) {
--- End diff --
Do you need the synchronized here if you're using Collections.synchronizedSet already?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108255892
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -109,6 +110,20 @@ public void channelConnected(ChannelHandlerContext ctx,
zkServer, NettyServerCnxnFactory.this);
ctx.setAttachment(cnxn);
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = ipMap.get(addr);
--- End diff --
Good catch! Congrats! :smiley:
Now, I am a bit unsure if this:
* this is a real issue and/or worth fixing;
* how to achieve a good compromise without entangling the logic too much.
Wdyt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108230180
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -524,17 +538,14 @@ public InetSocketAddress getLocalAddress() {
private void addCnxn(NettyServerCnxn cnxn) {
cnxns.add(cnxn);
- synchronized (ipMap){
- InetAddress addr =
- ((InetSocketAddress)cnxn.channel.getRemoteAddress())
- .getAddress();
- Set<NettyServerCnxn> s = ipMap.get(addr);
- if (s == null) {
- s = new HashSet<NettyServerCnxn>();
- }
- s.add(cnxn);
- ipMap.put(addr,s);
+
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
--- End diff --
I think it may be cleaner to inline this method instead of casting again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108258656
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -109,6 +110,20 @@ public void channelConnected(ChannelHandlerContext ctx,
zkServer, NettyServerCnxnFactory.this);
ctx.setAttachment(cnxn);
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = ipMap.get(addr);
--- End diff --
So I can't imagine this causing much more than the occasional extra single connection, but I do think it is worth fixing.
My feeling here is that if we move the ipMap logic to the factory this should be easier to fix as synchronization in a singleton is much simpler than trying to figure out what Netty does with these CnxnChannelHandler objects.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108230944
--- Diff: src/java/test/org/apache/zookeeper/server/NettyServerCnxnTest.java ---
@@ -84,4 +98,78 @@ public void testSendCloseSession() throws Exception {
zk.close();
}
}
+
+ @Test(timeout = 30000)
+ public void testMaxClientConnectionsReached() throws Exception {
+ final int maxClientCnxns = 4;
+ final int numClients = 10;
+ createAndTestConnections(numClients, maxClientCnxns, maxClientCnxns);
+ }
+
+ @Test(timeout = 30000)
+ public void testMaxClientConnectionsDisabled() throws Exception {
+ final int maxClientCnxns = 0; // disabled cnxns limit
+ final int numClients = 10;
+ createAndTestConnections(numClients, maxClientCnxns, numClients);
+ }
+
+ private void createAndTestConnections(int numClients, int maxClientCnxns, int cnxnsAccepted) throws Exception {
+
+ File tmpDir = ClientBase.createTmpDir();
+ final int CLIENT_PORT = PortAssignment.unique();
+
+ ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
+ ServerCnxnFactory scf = ServerCnxnFactory.createFactory(CLIENT_PORT, maxClientCnxns);
+ scf.startup(zks);
+
+ try {
+ assertTrue("waiting for server being up",
+ ClientBase.waitForServerUp("127.0.0.1:" + CLIENT_PORT, CONNECTION_TIMEOUT));
+ assertTrue("Didn't instantiate ServerCnxnFactory with NettyServerCnxnFactory!",
+ scf instanceof NettyServerCnxnFactory);
+
+ assertEquals(0, scf.getNumAliveConnections());
+
+ assertTrue(cnxnsAccepted <= numClients);
+
+ final CountDownLatch countDownLatch = new CountDownLatch(cnxnsAccepted);
+
+ TestableZooKeeper[] clients = new TestableZooKeeper[numClients];
+ for (int i = 0; i < numClients; i++) {
+ clients[i] = new TestableZooKeeper("127.0.0.1:" + CLIENT_PORT, 3000, new Watcher() {
+ @Override
+ public void process(WatchedEvent event)
+ {
+ if (event.getState() == Event.KeeperState.SyncConnected) {
+ countDownLatch.countDown();
+ }
+ }
+ });
+ }
+
+ countDownLatch.await();
+
+ assertEquals(cnxnsAccepted, scf.getNumAliveConnections());
+
+ ConcurrentMap<InetAddress, Set<NettyServerCnxn>> ipMap = ((NettyServerCnxnFactory) scf).ipMap;
+ assertEquals(1, ipMap.size());
+ Set<NettyServerCnxn> set = ipMap.get(ipMap.keySet().toArray()[0]);
+ assertEquals(cnxnsAccepted, set.size());
+
+ int connected = 0;
+ for (int i = 0; i < numClients; i++) {
+ if (clients[i].getState().isConnected()) connected++;
--- End diff --
nit: I think we prefer multi-line if statements.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108229036
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -109,6 +110,20 @@ public void channelConnected(ChannelHandlerContext ctx,
zkServer, NettyServerCnxnFactory.this);
ctx.setAttachment(cnxn);
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = ipMap.get(addr);
--- End diff --
Not sure if this is an issue or not, but if multiple connections (from a new address) hit this line before reaching the next one, the maxclientcnxns will never be checked as factory.ipMap.get(address) will return null for all of the connections?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108242277
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -109,6 +110,20 @@ public void channelConnected(ChannelHandlerContext ctx,
zkServer, NettyServerCnxnFactory.this);
ctx.setAttachment(cnxn);
+ InetAddress addr = ((InetSocketAddress)cnxn.channel.getRemoteAddress()).getAddress();
+ Set<NettyServerCnxn> s = ipMap.get(addr);
+ if (s != null) {
+ synchronized (s) {
--- End diff --
If we change `s` to be a synchronized set I think we can get rid of this synchronized block.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #208: ZOOKEEPER-2280: NettyServerCnxnFactory doesn't ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/208#discussion_r108225920
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -170,7 +185,6 @@ public void messageReceived(ChannelHandlerContext ctx, MessageEvent e)
throw ex;
}
}
-
--- End diff --
unnecessary line removal
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---