> On Dec. 15, 2014, 7:58 p.m., Gwen Shapira wrote: > > Jeff, > > > > There's some strange things going on there, I think we need a bit more > > testing and maybe more implementation :) > > > > 1. Please make sure the test fails without the override fix. When I tried, > > it passed on trunk... this means we are testing the wrong thing. > > > > 2. Funny fact: connect() does not actually trigger the Quota mechanism, it > > is only triggered when you send a request. You can see that by putting a > > breakpoint in ConnectionQuotas.inc and see where it is called. Since you > > are only sending data after creating the "last" connection, even without > > the override you'll be able to create the first 5 connections and only get > > the error after the 6 one and the "send request"... this is probably why > > the test works with and without the override. > > > > I'm not sure, but this may be a bug in the original maxIP implementation - > > since I can actually create gazillion connections as long as I don't send > > anything. I'm not sure if Kafka could run out of resources in this case. > > Perhaps check with Jay in the JIRA? He probably thought about this. > > > > 3. Not sure, but perhaps we need to call fail() explicitly to make sure the > > test fails if we successfully openned the last connection and sent data? > > > > 4. Another funny fact: ((0 until overrideNum).map(i => connect())) creates > > 6 connections, not 5 > > > > 5. We need to make sure the "overrides" map is propagated all the way to > > the ConnectionQuotas code. I don't think it does that at the moment, even > > after you fixed the SocketServer() call. > > > > Thanks again for your work here, and sorry it got slightly more complex > > than expected.
Gwen thanks for thew review. I will take a thorough look through the code and double check the tests. - Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29029/#review65114 ----------------------------------------------------------- On Dec. 15, 2014, 2:30 a.m., Jeff Holoman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29029/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2014, 2:30 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1512 > https://issues.apache.org/jira/browse/KAFKA-1512 > > > Repository: kafka > > > Description > ------- > > KAFKA-1512 wire in Override configuration > > > KAFKA-1512 wire in overrides > > > Diffs > ----- > > core/src/main/scala/kafka/network/SocketServer.scala > e451592fe358158548117f47a80e807007dd8b98 > core/src/main/scala/kafka/server/KafkaServer.scala > 1bf7d10cef23a77e716666eb16bf6d0e68bc4ebe > core/src/test/scala/unit/kafka/network/SocketServerTest.scala > 5f4d85254c384dcc27a5a84f0836ea225d3a901a > > Diff: https://reviews.apache.org/r/29029/diff/ > > > Testing > ------- > > > Thanks, > > Jeff Holoman > >
