> On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/network/RequestChannel.scala, line 48 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037026#file1037026line48> > > > > Normally one would use `Option[Session]` here. Are we using `null` due > > to efficiency concerns? Sorry if I am missing some context.
My bad, This class is not suppose to be part of this PR but the dependent jira. Removed this class. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Operation.scala, line 42 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037030#file1037030line42> > > > > Generally a good idea to set the result type for public methods. This > > makes it possible to change the underlying implementation without affecting > > binary compatibility. For example, here we may set the result type as > > `Seq[Operation]`, which would give us the option of changing the underlying > > implementation to `Vector` if that turned out to be better. In `Scala`, > > `List` is a concrete type unlike `Java`. > > > > Not sure what is the usual policy for Kafka though, would be useful to > > have some input from Jun. If we decide to change it, there are other places > > where the same comment would apply. > > Jun Rao wrote: > We don't have a policy on that yet. I think explicitly defining return > types in this case makes sense. Fixed in Operation, PermissionType and ResourceType. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Resource.scala, line 24 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037032#file1037032line24> > > > > Nitpick: space before `:` should be removed. Fixed. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 25 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037029#file1037029line25> > > > > Nitpick: space before `:` should be removed. Fixed. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 116 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037027#file1037027line116> > > > > Nitpick: no need for `()` and space before `:` should be removed. Fixed. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 104 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037034#file1037034line104> > > > > This code exists in 4 places, how about we introduce an method like: > > > > ``` > > def authorizeClusterAction(authorizer, request): Unit = { > > if (authorizer.map(_.authorizer(request.session, ClusterAction, > > Resource.ClusterResource)).getOrElse(false)) > > throw new AuthorizationException(s"Request $request is not > > authorized.") > > } > > ``` > > > > And then callers can just do (as an example): > > > > `authorizeClusterAction(authorizer, leaderAndIsrRequest)` > > > > > > Am I missing something? Fixed. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, lines 189-192 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037034#file1037034line189> > > > > Nitpick: space after `case`. There are a number of other cases like > > this. Fixed. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 549 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037034#file1037034line549> > > > > Nitpick: val instead of var. I am actually changing these values later so they need to be vars. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/test/scala/unit/kafka/security/auth/AclTest.scala, line 24 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037037#file1037037line24> > > > > We don't use `JUnit3Suite` anymore. Either use `JUnitSuite` or don't > > inherit from anything (we have both examples in the codebase now). This > > applies to all the tests. I noticed that Jun already mentioned this in > > another test. Fixed. > On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/test/scala/unit/kafka/security/auth/AclTest.scala, line 30 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037037#file1037037line30> > > > > @Test annotation is needed after you remove `JUnit3Suite`. This applies > > to all the tests. Fixed. - Parth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review95934 ----------------------------------------------------------- On Aug. 20, 2015, 6:27 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated Aug. 20, 2015, 6:27 p.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. > > > Addressing Jun's comments. > > > Merge remote-tracking branch 'origin/trunk' into az > > Conflicts: > core/src/main/scala/kafka/server/KafkaApis.scala > core/src/main/scala/kafka/server/KafkaServer.scala > > Deleting KafkaConfigDefTest > > > Addressing comments from Ismael. > > > 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 > 67f0cad802f901f255825aa2158545d7f5e7cc3d > core/src/main/scala/kafka/server/KafkaConfig.scala > d547a01cf7098f216a3775e1e1901c5794e1b24c > core/src/main/scala/kafka/server/KafkaServer.scala > 0e7ba3ede78dbc995c404e0387a6be687703836a > 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/KafkaConfigTest.scala > 3da666f73227fc7ef7093e3790546344065f6825 > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
