> On Jan. 16, 2015, 1:55 p.m., Eric Olander wrote: > > core/src/test/scala/unit/kafka/network/IPFilterTest.scala, line 71 > > <https://reviews.apache.org/r/29714/diff/2/?file=823240#file823240line71> > > > > This is more an FYI as it may go against established practices for > > tests in this project. Kafka includes scalatest which has a nice mechanism > > for this kind of testing: > > > > intercept[IPFilterConfigException] { > > new IPFilter(range, ruleType) > > } > > > > That does the same as the try/catch with the fail() in the try block.
Agreed. Based on some research I did when looking into KAFKA-1782 the testing guidleines are perhaps a bit in flux. I'm open to change this to whatever makes sense. > On Jan. 16, 2015, 1:55 p.m., Eric Olander wrote: > > core/src/main/scala/kafka/network/IPFilter.scala, line 38 > > <https://reviews.apache.org/r/29714/diff/2/?file=823235#file823235line38> > > > > Pattern matching is unnecessary here. Instead it is more > > performant/idiomatic to use the API calls on Option: > > > > if (rgx.findFirstIn(ruleType).isDefined) { > > ruleType > > } else { > > throw new IPFilterConfigException("Invalid rule type specified: " + > > ruleType) > > } Thanks for the Review! Will incorporate this. - Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29714/#review68424 ----------------------------------------------------------- On Jan. 16, 2015, 12:48 a.m., Jeff Holoman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29714/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2015, 12:48 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1810 > https://issues.apache.org/jira/browse/KAFKA-1810 > > > Repository: kafka > > > Description > ------- > > Put back in the 1512 changes and moved the initial BigInt out of the check > CIDRRange contains method > > > Diffs > ----- > > core/src/main/scala/kafka/network/IPFilter.scala PRE-CREATION > core/src/main/scala/kafka/network/SocketServer.scala > 39b1651b680b2995cedfde95d74c086d9c6219ef > core/src/main/scala/kafka/server/KafkaConfig.scala > 6e26c5436feb4629d17f199011f3ebb674aa767f > core/src/main/scala/kafka/server/KafkaServer.scala > 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b > core/src/main/scala/kafka/utils/VerifiableProperties.scala > 2ffc7f452dc7a1b6a06ca7a509ed49e1ab3d1e68 > core/src/test/scala/unit/kafka/network/IPFilterTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/network/SocketServerTest.scala > 78b431f9c88cca1bc5e430ffd41083d0e92b7e75 > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 2377abe4933e065d037828a214c3a87e1773a8ef > > Diff: https://reviews.apache.org/r/29714/diff/ > > > Testing > ------- > > This code centers around a new class, CIDRRange in IPFilter.scala. The > IPFilter class is created and holds two fields, the ruleType > (allow|deny|none) and a list of CIDRRange objects. This is used in the Socket > Server acceptor thread. The check does an exists on the value in the list if > the rule type is allow or deny. On object creation, we pre-calculate the > lower and upper range values and store those as a BigInt. The overhead of the > check should be fairly minimal as it involves converting the incoming IP > Address to a BigInt and then just doing a compare to the low/high values. In > writing this review up I realized that I can optimize this further to convert > to bigint first and move that conversion out of the range check, which I can > address. > > Testing covers the CIDRRange and IPFilter classes and validation of IPV6, > IPV4, and configurations. Additionally the functionality is tested in > SocketServerTest. Other changes are just to assist in configuration. > > I modified the SocketServerTest to use a method for grabbing the Socket > server to make the code a bit more concise. > > One key point is that, if there is an error in configuration, we halt the > startup of the broker. The thinking there is that if you screw up > security-related configs, you want to know about it right away rather than > silently accept connections. (thanks Joe Stein for the input). > > There are two new exceptions realted to this functionality, one to handle > configuration errors, and one to handle blocking the request. Currently the > level is set to INFO. Does it make sense to move this to WARN ? > > > Thanks, > > Jeff Holoman > >
