You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jay Kreps <bo...@gmail.com> on 2014/07/01 21:42:46 UTC
Review Request 23208: Patch for KAFKA-1512
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23208/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1512
https://issues.apache.org/jira/browse/KAFKA-1512
Repository: kafka
Description
-------
KAFKA-1512 Add per-ip connection limits.
Diffs
-----
core/src/main/scala/kafka/network/SocketServer.scala 4976d9c3a66bc965f5870a0736e21c7b32650bab
core/src/main/scala/kafka/server/KafkaConfig.scala ef75b67b67676ae5b8931902cbc8c0c2cc72c0d3
core/src/main/scala/kafka/server/KafkaServer.scala c22e51e0412843ec993721ad3230824c0aadd2ba
core/src/test/scala/unit/kafka/network/SocketServerTest.scala 62fb02cf02d3876b9804d756c4bf8514554cc836
Diff: https://reviews.apache.org/r/23208/diff/
Testing
-------
Thanks,
Jay Kreps
Re: Review Request 23208: Patch for KAFKA-1512
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23208/#review47151
-----------------------------------------------------------
Ship it!
Looks good to me, although the connection quotas will not yet work if we are still using sth. like a hardware load balancer or a VIP.
- Guozhang Wang
On July 1, 2014, 7:42 p.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23208/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 7:42 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1512
> https://issues.apache.org/jira/browse/KAFKA-1512
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1512 Add per-ip connection limits.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 4976d9c3a66bc965f5870a0736e21c7b32650bab
> core/src/main/scala/kafka/server/KafkaConfig.scala ef75b67b67676ae5b8931902cbc8c0c2cc72c0d3
> core/src/main/scala/kafka/server/KafkaServer.scala c22e51e0412843ec993721ad3230824c0aadd2ba
> core/src/test/scala/unit/kafka/network/SocketServerTest.scala 62fb02cf02d3876b9804d756c4bf8514554cc836
>
> Diff: https://reviews.apache.org/r/23208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 23208: Patch for KAFKA-1512
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23208/#review47841
-----------------------------------------------------------
Ship it!
Ship It!
- Jun Rao
On July 14, 2014, 8:28 p.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23208/
> -----------------------------------------------------------
>
> (Updated July 14, 2014, 8:28 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1512
> https://issues.apache.org/jira/browse/KAFKA-1512
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1512 Add per-ip connection limits.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 4976d9c3a66bc965f5870a0736e21c7b32650bab
> core/src/main/scala/kafka/server/KafkaConfig.scala bb2e654b9bd63daa88a4de14131859b75c00e607
> core/src/main/scala/kafka/server/KafkaServer.scala 5a56f57e36c4eab849a0b0e66f20f94690283af2
> core/src/test/scala/unit/kafka/network/SocketServerTest.scala 1c492de8fde6582ca2342842a551739575d1f46c
>
> Diff: https://reviews.apache.org/r/23208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 23208: Patch for KAFKA-1512
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23208/
-----------------------------------------------------------
(Updated July 14, 2014, 8:28 p.m.)
Review request for kafka.
Bugs: KAFKA-1512
https://issues.apache.org/jira/browse/KAFKA-1512
Repository: kafka
Description
-------
KAFKA-1512 Add per-ip connection limits.
Diffs (updated)
-----
core/src/main/scala/kafka/network/SocketServer.scala 4976d9c3a66bc965f5870a0736e21c7b32650bab
core/src/main/scala/kafka/server/KafkaConfig.scala bb2e654b9bd63daa88a4de14131859b75c00e607
core/src/main/scala/kafka/server/KafkaServer.scala 5a56f57e36c4eab849a0b0e66f20f94690283af2
core/src/test/scala/unit/kafka/network/SocketServerTest.scala 1c492de8fde6582ca2342842a551739575d1f46c
Diff: https://reviews.apache.org/r/23208/diff/
Testing
-------
Thanks,
Jay Kreps
Re: Review Request 23208: Patch for KAFKA-1512
Posted by Jay Kreps <bo...@gmail.com>.
> On July 6, 2014, 7:34 p.m., Jun Rao wrote:
> >
I think what I have may be right. My understanding is that
- InetAddress is an IP address
- SocketAddress, InetSocketAddress are host/port pairs (basically)
getInetAddress gets the remote ip address whereas getRemoteAddress gets the InetSocketAddress.
This is sort of crazy naming, but that is my understanding.
My thought was that it was better to work with InetAddress rather than host because host can be ambiguously defined (e.g. app301, app301.prod, app301.prod.linkedin.com, etc).
> On July 6, 2014, 7:34 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, lines 477-478
> > <https://reviews.apache.org/r/23208/diff/2/?file=623799#file623799line477>
> >
> > Would it be useful to add some metrics such as avg/max connections per host?
I think avg is not useful, and will be misleading in the case of a leak (e.g. one host with many, many connections). I think max would be useful, however it is a bit of a pain. The three options I could think of:
- add a gauge that iterates over all hosts in the map and computes the max. In the common case this will be fine, but it is O(N) in the number of hosts. So if you have a ton of connections and a low poll time for your metrics (some people use 1 sec) then you will spend something like 30% of one cpu core counting connections while holding the quota lock. In general I think we have tried to avoid operations that iterate over all hosts or connections to ensure the socket server scales to lots of connections without slowing down.
- Add a SortedSet on (count, ip) that gets adjusted on inc and dec. This is correct and O(1)ish but is just complicated.
- Implement a broken max that only goes up. This would be easily but annoying.
> On July 6, 2014, 7:34 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, lines 255-256
> > <https://reviews.apache.org/r/23208/diff/2/?file=623799#file623799line255>
> >
> > We probably should get remoteSocketAddress?
See above.
> On July 6, 2014, 7:34 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, lines 478-479
> > <https://reviews.apache.org/r/23208/diff/2/?file=623799#file623799line478>
> >
> > An InetAddress has a host and a port. It seems that we need to key on the hostName of an InetAddress.
See above.
> On July 6, 2014, 7:34 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 98-99
> > <https://reviews.apache.org/r/23208/diff/2/?file=623800#file623800line98>
> >
> > This may not work well with ipv6 since the ip will contain ":". This is also an issue for broker.list. So, do we want to address the issue now or fix it later together with broker.list?
This is a really good point. I don't care too much about this option as it is going to be rarely used. What I think is more important is that we fix the option in the new producer/consumer before we do an official release.
> On July 6, 2014, 7:34 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/network/SocketServerTest.scala, line 153
> > <https://reviews.apache.org/r/23208/diff/2/?file=623802#file623802line153>
> >
> > Should we just fail the test if we reach here?
Technically I think there is a race condition and either the exception or the read with -1 is possible, so I think you need both.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23208/#review47362
-----------------------------------------------------------
On July 3, 2014, 10:18 p.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23208/
> -----------------------------------------------------------
>
> (Updated July 3, 2014, 10:18 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1512
> https://issues.apache.org/jira/browse/KAFKA-1512
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1512 Add per-ip connection limits.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 4976d9c3a66bc965f5870a0736e21c7b32650bab
> core/src/main/scala/kafka/server/KafkaConfig.scala ef75b67b67676ae5b8931902cbc8c0c2cc72c0d3
> core/src/main/scala/kafka/server/KafkaServer.scala c22e51e0412843ec993721ad3230824c0aadd2ba
> core/src/test/scala/unit/kafka/network/SocketServerTest.scala 1c492de8fde6582ca2342842a551739575d1f46c
>
> Diff: https://reviews.apache.org/r/23208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 23208: Patch for KAFKA-1512
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23208/#review47362
-----------------------------------------------------------
core/src/main/scala/kafka/network/SocketServer.scala
<https://reviews.apache.org/r/23208/#comment83102>
We probably should get remoteSocketAddress?
core/src/main/scala/kafka/network/SocketServer.scala
<https://reviews.apache.org/r/23208/#comment83097>
Would it be useful to add some metrics such as avg/max connections per host?
core/src/main/scala/kafka/network/SocketServer.scala
<https://reviews.apache.org/r/23208/#comment83096>
An InetAddress has a host and a port. It seems that we need to key on the hostName of an InetAddress.
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/23208/#comment83100>
This may not work well with ipv6 since the ip will contain ":". This is also an issue for broker.list. So, do we want to address the issue now or fix it later together with broker.list?
core/src/test/scala/unit/kafka/network/SocketServerTest.scala
<https://reviews.apache.org/r/23208/#comment83098>
Should we just fail the test if we reach here?
core/src/test/scala/unit/kafka/network/SocketServerTest.scala
<https://reviews.apache.org/r/23208/#comment83099>
Do we really need to print the stack trace if this is expected?
- Jun Rao
On July 3, 2014, 10:18 p.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23208/
> -----------------------------------------------------------
>
> (Updated July 3, 2014, 10:18 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1512
> https://issues.apache.org/jira/browse/KAFKA-1512
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1512 Add per-ip connection limits.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 4976d9c3a66bc965f5870a0736e21c7b32650bab
> core/src/main/scala/kafka/server/KafkaConfig.scala ef75b67b67676ae5b8931902cbc8c0c2cc72c0d3
> core/src/main/scala/kafka/server/KafkaServer.scala c22e51e0412843ec993721ad3230824c0aadd2ba
> core/src/test/scala/unit/kafka/network/SocketServerTest.scala 1c492de8fde6582ca2342842a551739575d1f46c
>
> Diff: https://reviews.apache.org/r/23208/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 23208: Patch for KAFKA-1512
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23208/
-----------------------------------------------------------
(Updated July 3, 2014, 10:18 p.m.)
Review request for kafka.
Bugs: KAFKA-1512
https://issues.apache.org/jira/browse/KAFKA-1512
Repository: kafka
Description
-------
KAFKA-1512 Add per-ip connection limits.
Diffs (updated)
-----
core/src/main/scala/kafka/network/SocketServer.scala 4976d9c3a66bc965f5870a0736e21c7b32650bab
core/src/main/scala/kafka/server/KafkaConfig.scala ef75b67b67676ae5b8931902cbc8c0c2cc72c0d3
core/src/main/scala/kafka/server/KafkaServer.scala c22e51e0412843ec993721ad3230824c0aadd2ba
core/src/test/scala/unit/kafka/network/SocketServerTest.scala 1c492de8fde6582ca2342842a551739575d1f46c
Diff: https://reviews.apache.org/r/23208/diff/
Testing
-------
Thanks,
Jay Kreps