You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by lvfangmin <gi...@git.apache.org> on 2018/09/12 19:26:16 UTC
[GitHub] zookeeper pull request #623: [ZOOKEEPER-3146] Limit the maximum client conne...
GitHub user lvfangmin opened a pull request:
https://github.com/apache/zookeeper/pull/623
[ZOOKEEPER-3146] Limit the maximum client connections per IP in NettyServerCnxnFactory
Add similar max cnxn per ip throttling logic to Netty implementation.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lvfangmin/zookeeper ZOOKEEPER-3146
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/623.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 #623
----
commit 43bef3a6f91af7cc224122907e35b55d2d2db3c0
Author: Fangmin Lyu <al...@...>
Date: 2018-09-12T19:25:01Z
Limit the maximum client connections per IP in NettyServerCnxnFactory
----
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/623
retest this please
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/623
the latest jenkins failure is a known flaky test. merging this in..
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by dineshappavoo <gi...@git.apache.org>.
Github user dineshappavoo commented on the issue:
https://github.com/apache/zookeeper/pull/623
Hey @lvfangmin ,
This change looks good overall.
I am trying to involve in the community. I had a question about this change. I thought I would ask here. Please bare with me if this is very basic which I do not know :).
1. What is the difference between NIOServerCnxnFactory rate limiting and this one? What will be the impact after this change is merged?
Thanks.
---
[GitHub] zookeeper pull request #623: [ZOOKEEPER-3146] Limit the maximum client conne...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/623#discussion_r217450443
--- Diff: src/java/test/org/apache/zookeeper/test/ClientTest.java ---
@@ -506,7 +506,7 @@ private void performClientTest(boolean withWatcherObj)
}
--- End diff --
Yes, it's formatted when I was checking this test manually on my MAC (was trying to see where should I add the unit test), I'll disable the auto-formatting on my MAC.
The internal patches we did also have the white spaces formatted, we need to manually port the code line by line in some cases if we want to totally avoid the white spaces, I'll try my best, but I won't guarantee there is zero white space formatting.
---
[GitHub] zookeeper pull request #623: [ZOOKEEPER-3146] Limit the maximum client conne...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/623#discussion_r217585326
--- Diff: src/java/test/org/apache/zookeeper/test/ClientTest.java ---
@@ -506,7 +506,7 @@ private void performClientTest(boolean withWatcherObj)
}
--- End diff --
Thanks @lvfangmin, best effort is good enough, we can tolerate some white space change to avoid you have to port a patch line by line. As long as we are aware of the whitespace only change issue and try to avoid it, we will be good.
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/623
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2230/
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/623
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2232/
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/623
The [job I triggered on Sep 14](https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2168/) actually succeeded. It's the github integration that's broken. I suspect it's some permission issues around apache github mirror, I'll follow up with this as it's annoying.
I think this issue is good to land. If no more comments, I'll commit it on Friday.
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/623
retest this please
---
[GitHub] zookeeper pull request #623: [ZOOKEEPER-3146] Limit the maximum client conne...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/623
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/623
retest this please
---
[GitHub] zookeeper pull request #623: [ZOOKEEPER-3146] Limit the maximum client conne...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/623#discussion_r217260419
--- Diff: src/java/test/org/apache/zookeeper/test/ClientTest.java ---
@@ -506,7 +506,7 @@ private void performClientTest(boolean withWatcherObj)
}
--- End diff --
This file is not changed, except white spaces. Should we revert it?
A more general wish - is it possible to configure your editor such that it avoids rewriting white spaces (even technically your editor is correct in most cases)?
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/623
@hanm looks like the Jenkins job is pending there since you triggered it 5 days ago, it doesn't likely will be run, can you trigger it again?
---
[GitHub] zookeeper issue #623: [ZOOKEEPER-3146] Limit the maximum client connections ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/623
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2231/
---