subject:"\[jira\] \[Commented\] \(ZOOKEEPER\-2736\) Add a connection rate limiter"

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-04-03 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15953909#comment-15953909
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on the issue:

https://github.com/apache/zookeeper/pull/205
  
@vincentpoon Very cool, Vincent! :smiley: Sorry, I didn't have any free 
cycles to work on review comments regarding 
https://github.com/apache/zookeeper/pull/208 but hopefully gonna merge the 
changes tonight.

Best regards!


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-04-03 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15953880#comment-15953880
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user vincentpoon commented on the issue:

https://github.com/apache/zookeeper/pull/205
  
@eribeiro @afine Thanks for the reviews, put up an updated patch with your 
suggestions.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-31 Thread Hadoop QA (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15951835#comment-15951835
 ] 

Hadoop QA commented on ZOOKEEPER-2736:
--

-1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 10 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/495//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/495//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/495//console

This message is automatically generated.

> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15941242#comment-15941242
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r108010303
  
--- Diff: src/java/main/org/apache/zookeeper/common/TokenBucket.java ---
@@ -0,0 +1,88 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.apache.zookeeper.common;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Simple rate limiter based on a token bucket.
+  */
+public class TokenBucket implements RateLimiter {
+
+// VisibleForTesting
+long refreshPeriodNanos = TimeUnit.SECONDS.toNanos(1L);
+// VisibleForTesting
+volatile long nextRefillTime;
+private AtomicLong tokens;
+private long capacity;
+private long tokensPerPeriod;
+
+public TokenBucket() { }
+
+// VisibleForTesting
+public TokenBucket(long capacity, long tokensPerSecond, long 
initialTokens) {
+this.tokens = new AtomicLong(initialTokens);
+this.capacity = Math.max(capacity, tokensPerSecond);
+this.tokensPerPeriod = tokensPerSecond;
+this.nextRefillTime = System.nanoTime() + refreshPeriodNanos;
+}
+
+@Override
+public boolean tryAquire() {
+refill();
+long current = tokens.get();
+while (current > 0) {
+if (tokens.compareAndSet(current, current - 1)) {
+return true;
+}
+current = tokens.get();
+}
+return false;
+}
+
+private void refill() {
+long currentTime = System.nanoTime();
+if (currentTime >= nextRefillTime) {
+synchronized (this) {
+if (currentTime >= nextRefillTime) {
+long elapsedNanos = currentTime - nextRefillTime + 
refreshPeriodNanos;
+long elapsedPeriods = elapsedNanos / 
refreshPeriodNanos;
+long newTokens = elapsedPeriods * tokensPerPeriod;
+tokens.set(Math.min(capacity, tokens.get() + 
newTokens));
+nextRefillTime = currentTime + refreshPeriodNanos - 
(elapsedNanos % refreshPeriodNanos);
+}
+}
+}
+}
+
+// VisibleForTesting
+long getTokenCount() {
--- End diff --

+1.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15941241#comment-15941241
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r108009883
  
--- Diff: src/java/main/org/apache/zookeeper/common/TokenBucket.java ---
@@ -0,0 +1,88 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.apache.zookeeper.common;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Simple rate limiter based on a token bucket.
+  */
+public class TokenBucket implements RateLimiter {
+
+// VisibleForTesting
+long refreshPeriodNanos = TimeUnit.SECONDS.toNanos(1L);
+// VisibleForTesting
+volatile long nextRefillTime;
+private AtomicLong tokens;
+private long capacity;
+private long tokensPerPeriod;
+
+public TokenBucket() { }
+
+// VisibleForTesting
+public TokenBucket(long capacity, long tokensPerSecond, long 
initialTokens) {
+this.tokens = new AtomicLong(initialTokens);
+this.capacity = Math.max(capacity, tokensPerSecond);
+this.tokensPerPeriod = tokensPerSecond;
+this.nextRefillTime = System.nanoTime() + refreshPeriodNanos;
+}
+
+@Override
+public boolean tryAquire() {
--- End diff --

+1.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15941239#comment-15941239
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r108009773
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -845,6 +845,40 @@ server.3=zoo3:2888:3888

 

+rateLimiterImpl
+
+  (No Java system property)
+
+  Specifies an implementation of
+  org.apache.zookeeper.common.RateLimiter to use to limit the 
rate
+  at which a single client, identified by IP address, can make
+  connections to a single member of the ZooKeeper 
ensemble.
+
+   
+
+   
+maxClientCnxnRate
--- End diff --

+1 on that. :+1: 


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15940941#comment-15940941
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107973008
  
--- Diff: src/java/main/org/apache/zookeeper/common/TokenBucket.java ---
@@ -0,0 +1,88 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.apache.zookeeper.common;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Simple rate limiter based on a token bucket.
+  */
+public class TokenBucket implements RateLimiter {
+
+// VisibleForTesting
+long refreshPeriodNanos = TimeUnit.SECONDS.toNanos(1L);
+// VisibleForTesting
+volatile long nextRefillTime;
+private AtomicLong tokens;
+private long capacity;
+private long tokensPerPeriod;
+
+public TokenBucket() { }
+
+// VisibleForTesting
+public TokenBucket(long capacity, long tokensPerSecond, long 
initialTokens) {
+this.tokens = new AtomicLong(initialTokens);
+this.capacity = Math.max(capacity, tokensPerSecond);
+this.tokensPerPeriod = tokensPerSecond;
+this.nextRefillTime = System.nanoTime() + refreshPeriodNanos;
+}
+
+@Override
+public boolean tryAquire() {
+refill();
+long current = tokens.get();
+while (current > 0) {
+if (tokens.compareAndSet(current, current - 1)) {
+return true;
+}
+current = tokens.get();
+}
+return false;
+}
+
+private void refill() {
+long currentTime = System.nanoTime();
+if (currentTime >= nextRefillTime) {
+synchronized (this) {
+if (currentTime >= nextRefillTime) {
+long elapsedNanos = currentTime - nextRefillTime + 
refreshPeriodNanos;
+long elapsedPeriods = elapsedNanos / 
refreshPeriodNanos;
+long newTokens = elapsedPeriods * tokensPerPeriod;
+tokens.set(Math.min(capacity, tokens.get() + 
newTokens));
+nextRefillTime = currentTime + refreshPeriodNanos - 
(elapsedNanos % refreshPeriodNanos);
+}
+}
+}
+}
+
+// VisibleForTesting
+long getTokenCount() {
--- End diff --

nit: I think we generally prefer to explicitly state the access modifiers.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15940938#comment-15940938
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107965500
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -845,6 +845,40 @@ server.3=zoo3:2888:3888

 

+rateLimiterImpl
+
+  (No Java system property)
+
+  Specifies an implementation of
+  org.apache.zookeeper.common.RateLimiter to use to limit the 
rate
+  at which a single client, identified by IP address, can make
+  connections to a single member of the ZooKeeper 
ensemble.
+
+   
+
+   
+maxClientCnxnRate
--- End diff --

Would it make sense to tie this configuration to the implementation that is 
being used, rather than specifying at the top level what configuration is 
needed for the rate limiter?


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15940937#comment-15940937
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107976351
  
--- Diff: src/java/main/org/apache/zookeeper/common/TokenBucket.java ---
@@ -0,0 +1,88 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.apache.zookeeper.common;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Simple rate limiter based on a token bucket.
+  */
+public class TokenBucket implements RateLimiter {
+
+// VisibleForTesting
+long refreshPeriodNanos = TimeUnit.SECONDS.toNanos(1L);
+// VisibleForTesting
+volatile long nextRefillTime;
+private AtomicLong tokens;
+private long capacity;
+private long tokensPerPeriod;
+
+public TokenBucket() { }
+
+// VisibleForTesting
+public TokenBucket(long capacity, long tokensPerSecond, long 
initialTokens) {
+this.tokens = new AtomicLong(initialTokens);
+this.capacity = Math.max(capacity, tokensPerSecond);
+this.tokensPerPeriod = tokensPerSecond;
+this.nextRefillTime = System.nanoTime() + refreshPeriodNanos;
+}
+
+@Override
+public boolean tryAquire() {
--- End diff --

would things be easier if we just made this synchronized?


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15940940#comment-15940940
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107974911
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -39,6 +39,8 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.LinkedBlockingQueue;
 
+import org.apache.zookeeper.common.RateLimiter;
--- End diff --

+1 to this


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15940939#comment-15940939
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107974422
  
--- Diff: src/java/main/org/apache/zookeeper/common/TokenBucket.java ---
@@ -0,0 +1,88 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.apache.zookeeper.common;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Simple rate limiter based on a token bucket.
+  */
+public class TokenBucket implements RateLimiter {
+
+// VisibleForTesting
+long refreshPeriodNanos = TimeUnit.SECONDS.toNanos(1L);
+// VisibleForTesting
+volatile long nextRefillTime;
+private AtomicLong tokens;
+private long capacity;
+private long tokensPerPeriod;
+
+public TokenBucket() { }
+
+// VisibleForTesting
+public TokenBucket(long capacity, long tokensPerSecond, long 
initialTokens) {
+this.tokens = new AtomicLong(initialTokens);
+this.capacity = Math.max(capacity, tokensPerSecond);
+this.tokensPerPeriod = tokensPerSecond;
+this.nextRefillTime = System.nanoTime() + refreshPeriodNanos;
--- End diff --

nit: do we need nano seconds? can we just use 
org.apache.zookeeper.common.Time.currentElapsedTime() ?


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-24 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15940942#comment-15940942
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107973811
  
--- Diff: src/java/main/org/apache/zookeeper/common/RateLimiter.java ---
@@ -0,0 +1,99 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.zookeeper.common;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A connection rate limiter.
+ *
+ */
+public interface RateLimiter {
+
+/**
+ * If maxClientCnxnRate or maxClientCnxnBurst is set to this value, 
rate is
+ * not limited
+ */
+public static int BYPASS = -1;
+
+/**
+ * A {@code RateLimiter} that does not do any rate limiting
+ */
+public static RateLimiter BYPASS_RATE_LIMITER = new RateLimiter() {
+@Override
+public boolean tryAquire() {
+return true;
+}
+
+@Override
+public void configure(int maxClientCnxnRate, int 
maxClientCnxnBurst) {
+//not needed
+}
+};
+
+/**
+ * Attempts to acquire a permit
+ *
+ * @return true if a permit was acquired, false otherwise
+ */
+public boolean tryAquire();
+
+/**
+ * @param maxClientCnxnRate the max client connection rate
+ * @param maxClientCnxnBurst the max client connection burst
+ */
+public void configure(int maxClientCnxnRate, int maxClientCnxnBurst);
+
+public static class Factory {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(RateLimiter.Factory.class);
+
+/**
+ * Creates a {@code RateLimiter} with a stable average throughput 
of
+ * {@code averageRate} and a maximum burst size of {@code 
burstRate}
+ *
+ * @param rateLimiterImplClass the {@code RateLimiter} 
implementation to use
+ * @param burstSize
+ *The maximum burst size - i.e. max number of permits 
that
+ *can be acquired in a second
+ * @param averageRate
+ *The stable average rate in permits per second
+ * @return the {@code RateLimiter}
+ */
+public static RateLimiter create(String rateLimiterImplClass, int 
burstSize,
+int averageRate) {
+if (burstSize == BYPASS || averageRate == BYPASS) {
+return BYPASS_RATE_LIMITER;
+}
+if (rateLimiterImplClass == null) {
+rateLimiterImplClass = TokenBucket.class.getName();
+}
+try {
+RateLimiter limiter = (RateLimiter) 
Class.forName(rateLimiterImplClass).newInstance();
--- End diff --

it would be great to log which ratelimiter has been chosen


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939608#comment-15939608
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107820676
  
--- Diff: src/java/main/org/apache/zookeeper/common/RateLimiter.java ---
@@ -0,0 +1,99 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.zookeeper.common;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A connection rate limiter.
+ *
+ */
+public interface RateLimiter {
+
+/**
+ * If maxClientCnxnRate or maxClientCnxnBurst is set to this value, 
rate is
+ * not limited
+ */
+public static int BYPASS = -1;
+
+/**
+ * A {@code RateLimiter} that does not do any rate limiting
+ */
+public static RateLimiter BYPASS_RATE_LIMITER = new RateLimiter() {
+@Override
+public boolean tryAquire() {
+return true;
+}
+
+@Override
+public void configure(int maxClientCnxnRate, int 
maxClientCnxnBurst) {
+//not needed
--- End diff --

Nit: do nothing


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939601#comment-15939601
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107822984
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -750,4 +761,46 @@ public static void setReconfigEnabled(boolean enabled) 
{
 reconfigEnabled = enabled;
 }
 
+/**
+ * @return the {@code RateLimiter} to use
+ */
+public static String getRateLimiterImpl() {
--- End diff --

I am not so sure that creating a couple of static fields is the best 
approach here...


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939613#comment-15939613
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107820652
  
--- Diff: src/java/main/org/apache/zookeeper/common/RateLimiter.java ---
@@ -0,0 +1,99 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.zookeeper.common;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A connection rate limiter.
+ *
+ */
+public interface RateLimiter {
+
+/**
+ * If maxClientCnxnRate or maxClientCnxnBurst is set to this value, 
rate is
+ * not limited
+ */
+public static int BYPASS = -1;
+
+/**
+ * A {@code RateLimiter} that does not do any rate limiting
+ */
+public static RateLimiter BYPASS_RATE_LIMITER = new RateLimiter() {
--- End diff --

As above, no need for public static modifiers.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939607#comment-15939607
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107823274
  
--- Diff: src/java/test/org/apache/zookeeper/common/TokenBucketTest.java ---
@@ -0,0 +1,180 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.zookeeper.common;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.junit.Assert.*;
--- End diff --

IIRC, we prefer to use explicity imports instead of wildcards.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939604#comment-15939604
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107825629
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -607,9 +618,28 @@ public static ByteBuffer getDirectBuffer() {
 // sessionMap is used by closeSession()
 private final ConcurrentHashMap sessionMap =
 new ConcurrentHashMap();
-// ipMap is used to limit connections per IP
-private final ConcurrentHashMap ipMap 
=
-new ConcurrentHashMap( );
+// ipMap is used to limit connections and connection rate per IP
+private final ConcurrentHashMap ipMap
--- End diff --

If you are changing we can declare the field as `ConcurrentMap`.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939599#comment-15939599
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107824008
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -39,6 +39,8 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.LinkedBlockingQueue;
 
+import org.apache.zookeeper.common.RateLimiter;
--- End diff --

Where is the corresponding `NettyCnxnServerFactory`???


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939598#comment-15939598
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107822156
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -607,9 +618,28 @@ public static ByteBuffer getDirectBuffer() {
 // sessionMap is used by closeSession()
 private final ConcurrentHashMap sessionMap =
 new ConcurrentHashMap();
-// ipMap is used to limit connections per IP
-private final ConcurrentHashMap ipMap 
=
-new ConcurrentHashMap( );
+// ipMap is used to limit connections and connection rate per IP
+private final ConcurrentHashMap ipMap
+= new ConcurrentHashMap();
+
+// for each IP, we keep a RateLimiter to limit connection rate,
+// as well as a set of connections to limit total number of connections
+private static class IpCnxns {
+private IpCnxns(RateLimiter rateLimiter) {
+this.rateLimiter = rateLimiter;
+}
+
+// in general we will see 1 connection from each
+// host, setting the initial cap to 2 allows us
+// to minimize mem usage in the common case
+// of 1 entry -- we need to set the initial cap
+// to 2 to avoid rehash when the first entry is added
+// Construct a ConcurrentHashSet using a ConcurrentHashMap
+private Set cnxnSet = Collections
+.newSetFromMap(new ConcurrentHashMap(2));
+
+private RateLimiter rateLimiter;
--- End diff --

rateLimiter can be final, right? It doesn't change once passed to 
constructor.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939594#comment-15939594
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107820601
  
--- Diff: src/java/main/org/apache/zookeeper/common/RateLimiter.java ---
@@ -0,0 +1,99 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.zookeeper.common;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A connection rate limiter.
+ *
+ */
+public interface RateLimiter {
+
+/**
+ * If maxClientCnxnRate or maxClientCnxnBurst is set to this value, 
rate is
+ * not limited
+ */
+public static int BYPASS = -1;
--- End diff --

Fields in interfaces are implicitly public, static, and final. Declaring 
such keywords is redundant here


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939610#comment-15939610
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107825010
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -845,6 +845,40 @@ server.3=zoo3:2888:3888

 

+rateLimiterImpl
+
+  (No Java system property)
--- End diff --

Those params can be passed as System.properties. ;)


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939609#comment-15939609
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107821938
  
--- Diff: src/java/main/org/apache/zookeeper/common/TokenBucket.java ---
@@ -0,0 +1,88 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.apache.zookeeper.common;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Simple rate limiter based on a token bucket.
+  */
+public class TokenBucket implements RateLimiter {
+
+// VisibleForTesting
+long refreshPeriodNanos = TimeUnit.SECONDS.toNanos(1L);
+// VisibleForTesting
+volatile long nextRefillTime;
+private AtomicLong tokens;
+private long capacity;
+private long tokensPerPeriod;
+
+public TokenBucket() { }
+
+// VisibleForTesting
--- End diff --

Imo, it needs more api documentation, maybe a reference to a 
paper/book/wiki that explains the rationale behind this algorith.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939606#comment-15939606
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107820892
  
--- Diff: src/java/main/org/apache/zookeeper/common/RateLimiter.java ---
@@ -0,0 +1,99 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.zookeeper.common;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A connection rate limiter.
+ *
+ */
+public interface RateLimiter {
+
+/**
+ * If maxClientCnxnRate or maxClientCnxnBurst is set to this value, 
rate is
+ * not limited
+ */
+public static int BYPASS = -1;
+
+/**
+ * A {@code RateLimiter} that does not do any rate limiting
+ */
+public static RateLimiter BYPASS_RATE_LIMITER = new RateLimiter() {
+@Override
+public boolean tryAquire() {
+return true;
+}
+
+@Override
+public void configure(int maxClientCnxnRate, int 
maxClientCnxnBurst) {
+//not needed
+}
+};
+
+/**
+ * Attempts to acquire a permit
+ *
+ * @return true if a permit was acquired, false otherwise
+ */
+public boolean tryAquire();
+
+/**
+ * @param maxClientCnxnRate the max client connection rate
+ * @param maxClientCnxnBurst the max client connection burst
+ */
+public void configure(int maxClientCnxnRate, int maxClientCnxnBurst);
+
+public static class Factory {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(RateLimiter.Factory.class);
+
+/**
+ * Creates a {@code RateLimiter} with a stable average throughput 
of
+ * {@code averageRate} and a maximum burst size of {@code 
burstRate}
+ *
+ * @param rateLimiterImplClass the {@code RateLimiter} 
implementation to use
--- End diff --

Please, specify that if this String is null then a TokenBucket will be 
chosen by default. By the way, I am not so sure if BYPASS_RATE_LIMITER would 
not be better in this case... :thinking:


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939605#comment-15939605
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107825073
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -845,6 +845,40 @@ server.3=zoo3:2888:3888

 

+rateLimiterImpl
+
+  (No Java system property)
+
+  Specifies an implementation of
+  org.apache.zookeeper.common.RateLimiter to use to limit the 
rate
+  at which a single client, identified by IP address, can make
+  connections to a single member of the ZooKeeper 
ensemble.
+
+   
+
+   
+maxClientCnxnRate
+
+  (No Java system property)
--- End diff --

`zookeeper.maxClientCnxnRate`


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939602#comment-15939602
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107823091
  
--- Diff: src/java/test/org/apache/zookeeper/test/MaxCnxnRateTest.java ---
@@ -0,0 +1,124 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.zookeeper.test;
+
+import static org.junit.Assert.assertEquals;
+
+import java.util.ArrayList;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.zookeeper.common.TokenBucket;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
+import org.apache.zookeeper.test.MaxCnxnsTest.CnxnThread;
+import org.junit.Test;
+
+public class MaxCnxnRateTest extends ClientBase {
+
+private int maxClientCnxnRate = 1;
+private int maxClientCnxnBurst = 10;
+private String host;
+private int port;
+static CountDownLatch startLatch;
+
+@Override
+public void startServer() throws Exception {
+LOG.info("STARTING server");
+maxCnxns = 1000;
+String split[] = hostPort.split(":");
+host = split[0];
+port = Integer.parseInt(split[1]);
+QuorumPeerConfig.setRateLimiterImpl(TokenBucket.class.getName());
+QuorumPeerConfig.setClientCnxnRate(maxClientCnxnRate);
+QuorumPeerConfig.setClientCnxnBurst(maxClientCnxnBurst);
+serverFactory = ServerCnxnFactory.createFactory(port, maxCnxns);
+startServer(serverFactory);
+startLatch = new CountDownLatch(1);
+}
+
+static class LatchedCnxnThread extends CnxnThread {
+
+public LatchedCnxnThread(int i, String host, int port, 
AtomicInteger connectionCounter) {
+super(i, host, port, connectionCounter);
+}
+
+@Override
+public void run() {
+try {
+startLatch.await();
+super.run();
+
+} catch (InterruptedException e) {
+e.printStackTrace();
+}
+}
+}
+
+/**
+ * Creates more connection threads than our burst size.  All try to 
connect
+ * simultaneously.  Ensures that only the burst size succeeded.
+ */
+@Test
+public void testMaxCnxnBurst() throws InterruptedException {
+AtomicInteger connectionCounter = new AtomicInteger(0);
+ArrayList threads = new 
ArrayList<>(maxClientCnxnBurst+5);
+for (int i = 0; i < maxClientCnxnBurst+5; i++) {
+LatchedCnxnThread thread = new LatchedCnxnThread(i, host, 
port, connectionCounter);
+thread.start();
+threads.add(thread);
+}
+
+startLatch.countDown();
+
+for (LatchedCnxnThread thread : threads) {
+thread.join();
+}
+assertEquals(maxClientCnxnBurst, connectionCounter.get());
+}
+
+/**
+ * Creates connections in a tight loop, and ensures rate is limited
+ */
+@Test
+public void testMaxCnxnRate() throws InterruptedException {
+AtomicInteger connectionCounter = new AtomicInteger(0);
+int i = 0;
+ArrayList threads = new ArrayList<>(300);
--- End diff --

Nit: `List threads = ...`


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939612#comment-15939612
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107825507
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -607,9 +618,28 @@ public static ByteBuffer getDirectBuffer() {
 // sessionMap is used by closeSession()
 private final ConcurrentHashMap sessionMap =
 new ConcurrentHashMap();
-// ipMap is used to limit connections per IP
-private final ConcurrentHashMap ipMap 
=
-new ConcurrentHashMap( );
+// ipMap is used to limit connections and connection rate per IP
+private final ConcurrentHashMap ipMap
+= new ConcurrentHashMap();
+
+// for each IP, we keep a RateLimiter to limit connection rate,
+// as well as a set of connections to limit total number of connections
+private static class IpCnxns {
+private IpCnxns(RateLimiter rateLimiter) {
+this.rateLimiter = rateLimiter;
+}
+
+// in general we will see 1 connection from each
+// host, setting the initial cap to 2 allows us
+// to minimize mem usage in the common case
+// of 1 entry -- we need to set the initial cap
+// to 2 to avoid rehash when the first entry is added
+// Construct a ConcurrentHashSet using a ConcurrentHashMap
+private Set cnxnSet = Collections
--- End diff --

Please, make cnxnSet final.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939611#comment-15939611
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107824547
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -607,9 +618,28 @@ public static ByteBuffer getDirectBuffer() {
 // sessionMap is used by closeSession()
 private final ConcurrentHashMap sessionMap =
 new ConcurrentHashMap();
-// ipMap is used to limit connections per IP
-private final ConcurrentHashMap ipMap 
=
-new ConcurrentHashMap( );
+// ipMap is used to limit connections and connection rate per IP
+private final ConcurrentHashMap ipMap
+= new ConcurrentHashMap();
+
+// for each IP, we keep a RateLimiter to limit connection rate,
+// as well as a set of connections to limit total number of connections
+private static class IpCnxns {
--- End diff --

Eventually, we need to implement same logic for NettyServerCnxnFactory so 
better to up this class to ServerCnxnFactory or make it a top level class. The 
former is fine, imho.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939595#comment-15939595
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107821391
  
--- Diff: src/java/test/org/apache/zookeeper/test/MaxCnxnRateTest.java ---
@@ -0,0 +1,124 @@
+/**
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.zookeeper.test;
+
+import static org.junit.Assert.assertEquals;
+
+import java.util.ArrayList;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.zookeeper.common.TokenBucket;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
+import org.apache.zookeeper.test.MaxCnxnsTest.CnxnThread;
+import org.junit.Test;
+
+public class MaxCnxnRateTest extends ClientBase {
+
+private int maxClientCnxnRate = 1;
+private int maxClientCnxnBurst = 10;
+private String host;
+private int port;
+static CountDownLatch startLatch;
+
+@Override
+public void startServer() throws Exception {
+LOG.info("STARTING server");
+maxCnxns = 1000;
+String split[] = hostPort.split(":");
+host = split[0];
+port = Integer.parseInt(split[1]);
+QuorumPeerConfig.setRateLimiterImpl(TokenBucket.class.getName());
+QuorumPeerConfig.setClientCnxnRate(maxClientCnxnRate);
+QuorumPeerConfig.setClientCnxnBurst(maxClientCnxnBurst);
+serverFactory = ServerCnxnFactory.createFactory(port, maxCnxns);
+startServer(serverFactory);
+startLatch = new CountDownLatch(1);
+}
+
+static class LatchedCnxnThread extends CnxnThread {
+
+public LatchedCnxnThread(int i, String host, int port, 
AtomicInteger connectionCounter) {
+super(i, host, port, connectionCounter);
+}
+
+@Override
+public void run() {
+try {
+startLatch.await();
+super.run();
+
--- End diff --

Unnecessary blank line.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939596#comment-15939596
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107824726
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -794,8 +825,9 @@ public boolean removeCnxn(NIOServerCnxn cnxn) {
 
 InetAddress addr = cnxn.getSocketAddress();
 if (addr != null) {
-Set set = ipMap.get(addr);
-if (set != null) {
+IpCnxns ipCnxns = ipMap.get(addr);
+if (ipCnxns != null) {
+Set set = ipCnxns.cnxnSet;
--- End diff --

Direct field access... I see ipCnxns is internal... But awkward yet. :smile:


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939603#comment-15939603
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/205#discussion_r107824798
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -817,24 +849,23 @@ public void touchCnxn(NIOServerCnxn cnxn) {
 
 private void addCnxn(NIOServerCnxn cnxn) {
 InetAddress addr = cnxn.getSocketAddress();
-Set set = ipMap.get(addr);
-if (set == null) {
-// in general we will see 1 connection from each
-// host, setting the initial cap to 2 allows us
-// to minimize mem usage in the common case
-// of 1 entry --  we need to set the initial cap
-// to 2 to avoid rehash when the first entry is added
-// Construct a ConcurrentHashSet using a ConcurrentHashMap
-set = Collections.newSetFromMap(
-new ConcurrentHashMap(2));
-// Put the new set in the map, but only if another thread
+IpCnxns ipCnxns = ipMap.get(addr);
+
+if (ipCnxns == null) {
+// create an IpCnxns which is a wrapper that holds a 
RateLimiter and
+// a set of connections for each ip
+RateLimiter rateLimiter = RateLimiter.Factory.create(
+QuorumPeerConfig.getRateLimiterImpl(), 
QuorumPeerConfig.getClientCnxnBurst(),
+QuorumPeerConfig.getClientCnxnRate());
+ipCnxns = new IpCnxns(rateLimiter);
+// Put the ip limiter/set in the map, but only if another 
thread
 // hasn't beaten us to it
-Set existingSet = ipMap.putIfAbsent(addr, set);
-if (existingSet != null) {
-set = existingSet;
+IpCnxns existingIpCnxns = ipMap.putIfAbsent(addr, ipCnxns);
+if (existingIpCnxns != null) {
+ipCnxns = existingIpCnxns;
 }
 }
-set.add(cnxn);
+ipCnxns.cnxnSet.add(cnxn);
 
--- End diff --

Nit: direct field access... again.


> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread Hadoop QA (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939537#comment-15939537
 ] 

Hadoop QA commented on ZOOKEEPER-2736:
--

+1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 10 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/476//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/476//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/476//console

This message is automatically generated.

> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread Hadoop QA (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939459#comment-15939459
 ] 

Hadoop QA commented on ZOOKEEPER-2736:
--

-1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 10 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/475//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/475//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/475//console

This message is automatically generated.

> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread Hadoop QA (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939348#comment-15939348
 ] 

Hadoop QA commented on ZOOKEEPER-2736:
--

-1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 10 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

-1 findbugs.  The patch appears to introduce 1 new Findbugs (version 3.0.1) 
warnings.

-1 release audit.  The applied patch generated 1 release audit warnings 
(more than the trunk's current 0 warnings).

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/474//testReport/
Release audit warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/474//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/474//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/474//console

This message is automatically generated.

> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939307#comment-15939307
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2736:
---

GitHub user vincentpoon opened a pull request:

https://github.com/apache/zookeeper/pull/205

ZOOKEEPER-2736 Add a connection rate limiter



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/vincentpoon/zookeeper ZOOKEEPER-2736

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/205.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 #205


commit fcbc77c41f0dd71035c3f7426a7c63e7a75cb416
Author: Vincent 
Date:   2017-03-23T22:11:57Z

ZOOKEEPER-2736 Add a connection rate limiter




> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

2017-03-23 Thread Edward Ribeiro (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15939052#comment-15939052
 ] 

Edward Ribeiro commented on ZOOKEEPER-2736:
---

Hey, [~vincentpoon], nice initiative!

I remember some past discussion about introducing a rate limiter into ZK 
codebase, particularly in the context of ZOOKEEPER-2693, so you may have to 
take a look there first to see what the others contributors have in mind. Also, 
*and most important*, we are not using patch upload + review board to accept 
patches in ZooKeeper anymore. Please, open a PR on Github as specified here: 
https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute

Best regards

> Add a connection rate limiter
> -
>
> Key: ZOOKEEPER-2736
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2736
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: server
>Affects Versions: 3.4.9, 3.5.2
>Reporter: Vincent Poon
> Attachments: ZOOKEEPER-2736.v1.patch
>
>
> Currently the maxClientCnxns property only limits the aggregate number of 
> connections from a client, but not the rate at which connections can be 
> created.
> This patch adds a configurable connection rate limiter which limits the rate 
> as well.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

[jira] [Commented] (ZOOKEEPER-2736) Add a connection rate limiter

35 matches

Site Navigation

Mail list logo

Footer information