----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29714/#review68424 -----------------------------------------------------------
core/src/main/scala/kafka/network/IPFilter.scala <https://reviews.apache.org/r/29714/#comment112638> 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) } core/src/test/scala/unit/kafka/network/IPFilterTest.scala <https://reviews.apache.org/r/29714/#comment112639> 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. - Eric Olander 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 > >
