> On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/common/AuthorizationException.scala, line 24 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018318#file1018318line24> > > > > Exceptions without a message are discouraged, so I would remove the > > no-args constructor.
fixed. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Authorizer.scala, line 64 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018321#file1018321line64> > > > > Should this be called `removeResource` instead? Good to avoid method > > overloads when possible. its actually not removing the resource itself though, it is only removing acls attached to the acls. Want to avoid naming it somehting that will be misleading. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 26 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018322#file1018322line26> > > > > Type annotations are usually not used for local vals. done for all classes under auth package. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 28 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018322#file1018322line28> > > > > Code contention: no braces for single expression. again done for all classed under auth package. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 32 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018322#file1018322line32> > > > > 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(...) > > } > > ``` made changes at KafkaPrincipal and Resource. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 41 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018322#file1018322line41> > > > > 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. I have changed all the classes to case class. This basically means all the equalities are now case sensitive. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Operation.scala, line 38 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018323#file1018323line38> > > > > `find` is better than `filter` here as it stops once the match is > > found. Also, `headOption` is not needed then. fixed. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Operation.scala, line 42 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018323#file1018323line42> > > > > Code convention: no braces needed for single expression. > > > > Also, no need for `()` since there is no side-effect here. fixed. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/PermissionType.scala, line 38 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018324#file1018324line38> > > > > Same points as the ones in `Operation`. fixed. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Resource.scala, line 22 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018325#file1018325line22> > > > > Space after `,`. fixed. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Resource.scala, line 31 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018325#file1018325line31> > > > > Same comments as in `KafkaPrincipal.fromString`. fixed. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Resource.scala, line 41 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018325#file1018325line41> > > > > Make it a case class? mixed. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/ResourceType.scala, line 52 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018326#file1018326line52> > > > > Same comments as in `Operation`. fixed. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/test/scala/unit/kafka/security/auth/AclTest.scala, line 44 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018330#file1018330line44> > > > > Type annotation is generally not needed for local `vals` (there are a > > number of instances of this). Fixed all instances that I could find. > On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 104 > > <https://reviews.apache.org/r/34492/diff/9/?file=1018327#file1018327line104> > > > > 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. done. - Parth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review93302 ----------------------------------------------------------- On Aug. 11, 2015, 1:32 a.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2015, 1:32 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. > > > Now addressing Ismael's comments. Case sensitive checks. > > > 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/network/RequestChannel.scala > 20741281dcaa76374ea6f86a2185dad27b515339 > 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 > 7ea509c2c41acc00430c74e025e069a833aac4e7 > core/src/main/scala/kafka/server/KafkaConfig.scala > dbe170f87331f43e2dc30165080d2cb7dfe5fdbf > core/src/main/scala/kafka/server/KafkaServer.scala > 84d4730ac634f9a5bf12a656e422fea03ad72da8 > 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/ResourceTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
