----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34493/#review96042 -----------------------------------------------------------
Thanks for this Parth. I did an initial pass where I left a number comments (many of them style-related, see http://kafka.apache.org/coding-guide.html for reference). I know, we should have a tool that checks some of these things automatically. That is my main priority after we get the security stuff in shape. I think it would be useful if you had a look and made the changes (if you agree) to the cases I pointed out and similar ones. I noticed that KAFKA-2212 has some similar issues too, it may be worth taking a pass there too. I will look at these two patches again early next week. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 53) <https://reviews.apache.org/r/34493/#comment151231> `str` != null is not needed because that was already checked previously in `case null`. Also, if `superUser` is a String we should not check if it's a String again by doing `case (str: String)`. If it is not, then we need to handle the case where it's something else. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 66) <https://reviews.apache.org/r/34493/#comment151232> Nitpick: space after `if` (there are other cases like this). core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 79) <https://reviews.apache.org/r/34493/#comment151229> This seems a bit odd. Do we really want to `authorize` in this case? I'd think we want to throw an exception or return `false`, no? I'd probably go wikth the exception. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 82) <https://reviews.apache.org/r/34493/#comment151230> We don't normally use type annotations in local vals for obvious cases. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 100) <https://reviews.apache.org/r/34493/#comment151233> Replace `if/else` by `&&`, no? core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 114) <https://reviews.apache.org/r/34493/#comment151234> Nitpick: No braces needed for single expression. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 118) <https://reviews.apache.org/r/34493/#comment151235> In Scala, `return` is normally avoided because it makes it harder to reason about some code in isolation. Could this code be rewritten in a more readable way without using `return`? The answer may well be no, interested in your thoughts. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 146) <https://reviews.apache.org/r/34493/#comment151236> `inReadLock` returns a value. No need for the `var` above. - Ismael Juma On May 20, 2015, 8:03 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34493/ > ----------------------------------------------------------- > > (Updated May 20, 2015, 8:03 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2211 > https://issues.apache.org/jira/browse/KAFKA-2211 > > > Repository: kafka > > > Description > ------- > > KAFKA-2211: Out of box implementation for authorizer interface. > > > Diffs > ----- > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala > PRE-CREATION > core/src/test/resources/authorizer-config.properties PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/SimpleAclAuthorizerTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/34493/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
