> On Aug. 21, 2015, 2:31 p.m., Ismael Juma wrote: > > 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. > > Parth Brahmbhatt wrote: > Hey , thanks for reviewing this. This patch needs to be updated with all > the changes that we have made in 2210. As 2210 was moving slow and was kind > of a moving target I did not update this patch. I think I have gained little > more understanding around scala styles from the 2210 review so I will > incorporate all those in this patch. > > Ismael Juma wrote: > Thank you Parth!
Is it ok if I post a pull request with the new changes instead of using review board? > On Aug. 21, 2015, 2:31 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala, line 118 > > <https://reviews.apache.org/r/34493/diff/1/?file=965666#file965666line118> > > > > 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. I think I have addressed all your comments but this one. This seems one of those cases where the code will be more redable with return statement given there are so many different checks and at end of each of those checks we might have found the final result. - Parth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34493/#review96042 ----------------------------------------------------------- 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 > >
