Re: Review Request 23208: Patch for KAFKA-1512
--- 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
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
--- 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
--- 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
--- 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
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
--- 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
