----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review93302 -----------------------------------------------------------
I did an initial pass and left some comments. Some of them are questions and a lot of them are around idiomatic Scala. I didn't go until the end because I was seeing similar issues to the ones I had already raised and I thought it would be better to wait for feedback before continuing. core/src/main/scala/kafka/common/AuthorizationException.scala (line 24) <https://reviews.apache.org/r/34492/#comment147676> Exceptions without a message are discouraged, so I would remove the no-args constructor. core/src/main/scala/kafka/security/auth/Authorizer.scala (line 64) <https://reviews.apache.org/r/34492/#comment147678> Should this be called `removeResource` instead? Good to avoid method overloads when possible. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line 26) <https://reviews.apache.org/r/34492/#comment147679> Type annotations are usually not used for local vals. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line 28) <https://reviews.apache.org/r/34492/#comment147680> Code contention: no braces for single expression. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line 32) <https://reviews.apache.org/r/34492/#comment147681> This could be written in a nicer way like this: ``` str.split(Separator, 2) match { case Array(principalType, name, _*) => new KafkaPrincipal(principalType, name) case s => throw IllegalArgumentException(...) } ``` core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line 41) <https://reviews.apache.org/r/34492/#comment147682> If you make this a case class, you get decent `toString`, `equals` and `hashCode` by default. Also, the `val` is then not needed for the fields. core/src/main/scala/kafka/security/auth/Operation.scala (line 38) <https://reviews.apache.org/r/34492/#comment147684> `find` is better than `filter` here as it stops once the match is found. Also, `headOption` is not needed then. core/src/main/scala/kafka/security/auth/Operation.scala (line 42) <https://reviews.apache.org/r/34492/#comment147683> Code convention: no braces needed for single expression. Also, no need for `()` since there is no side-effect here. core/src/main/scala/kafka/security/auth/PermissionType.scala (line 38) <https://reviews.apache.org/r/34492/#comment147685> Same points as the ones in `Operation`. core/src/main/scala/kafka/security/auth/Resource.scala (line 22) <https://reviews.apache.org/r/34492/#comment147687> Space after `,`. core/src/main/scala/kafka/security/auth/Resource.scala (line 31) <https://reviews.apache.org/r/34492/#comment147686> Same comments as in `KafkaPrincipal.fromString`. core/src/main/scala/kafka/security/auth/Resource.scala (line 41) <https://reviews.apache.org/r/34492/#comment147688> Make it a case class? core/src/main/scala/kafka/security/auth/ResourceType.scala (line 52) <https://reviews.apache.org/r/34492/#comment147689> Same comments as in `Operation`. core/src/main/scala/kafka/server/KafkaApis.scala (line 101) <https://reviews.apache.org/r/34492/#comment147691> In general, `Option.get` should be avoided. Instead of manually checking if it's defined, use safer methods. For example: ```authorizer.foreach { a => if (!a.authorize(...) throw new ... ``` `filter`/`filterNot` could also be used before `foreach` instead of the `if`, but it doesn't give much in this case. core/src/main/scala/kafka/server/KafkaApis.scala (line 164) <https://reviews.apache.org/r/34492/#comment147692> This code is duplicated in a number of places, can we not extract it into a method? core/src/main/scala/kafka/server/KafkaApis.scala (line 185) <https://reviews.apache.org/r/34492/#comment147695> Use `case` like in the `requestInfo.filter` call to make the code more readable. Also good to avoid `Option.get` as mentioned above. core/src/main/scala/kafka/server/KafkaApis.scala (line 341) <https://reviews.apache.org/r/34492/#comment147696> Use `map` or `fold` instead of `Option.get`. core/src/test/scala/unit/kafka/security/auth/AclTest.scala (line 44) <https://reviews.apache.org/r/34492/#comment147698> Type annotation is generally not needed for local `vals` (there are a number of instances of this). - Ismael Juma On July 22, 2015, 12:08 a.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated July 22, 2015, 12:08 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2210 > https://issues.apache.org/jira/browse/KAFKA-2210 > > > Repository: kafka > > > Description > ------- > > Addressing review comments from Jun. > > > Adding CREATE check for offset topic only if the topic does not exist already. > > > Addressing some more comments. > > > Removing acl.json file > > > Moving PermissionType to trait instead of enum. Following the convention for > defining constants. > > > Adding authorizer.config.path back. > > > Addressing more comments from Jun. > > > Addressing more comments. > > > Diffs > ----- > > core/src/main/scala/kafka/api/OffsetRequest.scala > f418868046f7c99aefdccd9956541a0cb72b1500 > core/src/main/scala/kafka/common/AuthorizationException.scala PRE-CREATION > core/src/main/scala/kafka/common/ErrorMapping.scala > c75c68589681b2c9d6eba2b440ce5e58cddf6370 > core/src/main/scala/kafka/security/auth/Acl.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Authorizer.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Operation.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/PermissionType.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Resource.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/ResourceType.scala PRE-CREATION > core/src/main/scala/kafka/server/KafkaApis.scala > 18f5b5b895af1469876b2223841fd90a2dd255e0 > core/src/main/scala/kafka/server/KafkaConfig.scala > dbe170f87331f43e2dc30165080d2cb7dfe5fdbf > core/src/main/scala/kafka/server/KafkaServer.scala > 18917bc4464b9403b16d85d20c3fd4c24893d1d3 > core/src/test/scala/unit/kafka/security/auth/AclTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/KafkaPrincipalTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/OperationTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/PermissionTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/ResourceTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/ResourceTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > 04a02e08a54139ee1a298c5354731bae009efef3 > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
