-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34492/#review95801
-----------------------------------------------------------
Thanks for the patch. A few comments below. There is a unit test failure.
kafka.server.KafkaConfigTest > testFromPropsInvalid FAILED
org.scalatest.junit.JUnitTestFailedError: Expected exception
java.lang.Exception to be thrown, but no exception was thrown.
at
org.scalatest.junit.AssertionsForJUnit$class.newAssertionFailedException(AssertionsForJUnit.scala:102)
at
org.scalatest.junit.JUnit3Suite.newAssertionFailedException(JUnit3Suite.scala:147)
at org.scalatest.Assertions$class.intercept(Assertions.scala:1014)
at org.scalatest.junit.JUnit3Suite.intercept(JUnit3Suite.scala:147)
at
kafka.server.KafkaConfigTest$$anonfun$kafka$server$KafkaConfigTest$$assertPropertyInvalid$1.apply(KafkaConfigTest.scala:529)
at
kafka.server.KafkaConfigTest$$anonfun$kafka$server$KafkaConfigTest$$assertPropertyInvalid$1.apply(KafkaConfigTest.scala:526)
at
scala.collection.IndexedSeqOptimized$class.foreach(IndexedSeqOptimized.scala:33)
at scala.collection.mutable.WrappedArray.foreach(WrappedArray.scala:34)
at
kafka.server.KafkaConfigTest.kafka$server$KafkaConfigTest$$assertPropertyInvalid(KafkaConfigTest.scala:526)
at
kafka.server.KafkaConfigTest$$anonfun$testFromPropsInvalid$1.apply(KafkaConfigTest.scala:484)
at
kafka.server.KafkaConfigTest$$anonfun$testFromPropsInvalid$1.apply(KafkaConfigTest.scala:395)
at scala.collection.immutable.List.foreach(List.scala:318)
at
kafka.server.KafkaConfigTest.testFromPropsInvalid(KafkaConfigTest.scala:395)
core/src/main/scala/kafka/security/auth/Acl.scala (lines 84 - 87)
<https://reviews.apache.org/r/34492/#comment150944>
Do we need the case match here since acls is always a Set?
core/src/main/scala/kafka/security/auth/ResourceType.scala (line 21)
<https://reviews.apache.org/r/34492/#comment151082>
Not needed.
core/src/main/scala/kafka/server/KafkaApis.scala (line 101)
<https://reviews.apache.org/r/34492/#comment151087>
If authorizer is not specified, getOrElse() should return true, right?
There are a few other places like that.
core/src/main/scala/kafka/server/KafkaApis.scala (lines 186 - 189)
<https://reviews.apache.org/r/34492/#comment151098>
For coding style, to be consistent with most existing code, it seems it's
better to do authorizer.map{ ... }.getOrElse().
core/src/main/scala/kafka/server/KafkaApis.scala (line 549)
<https://reviews.apache.org/r/34492/#comment151097>
Currently, metadataCache.getTopicMetadata() will return all the metadata of
all topics if the input topic is empty. This causes a couple of issues here.
(1) If authorizedTopics is empty, we end up returning more topics than needed.
(2) If the original request has an empty topic list, we will return the
metadata of all topics whether the client has the DESCRIBE permission or not.
core/src/main/scala/kafka/server/KafkaApis.scala (lines 637 - 640)
<https://reviews.apache.org/r/34492/#comment151099>
The current logic requires that we grant CREATE on CLUSTER to the consumer,
which is a bit weird. Perhaps we should just always allow the consumer to
create the offset topic as long as it has the permission to read the topic and
the consumer group. That way, we don't have to grant CREATE permssion to the
consumer.
core/src/main/scala/kafka/server/KafkaApis.scala (line 673)
<https://reviews.apache.org/r/34492/#comment151100>
It seems the test on errorCode == ErrorMapping.NoError is unnecessary.
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala (line 30)
<https://reviews.apache.org/r/34492/#comment151101>
We removed this class in KAFKA-2288 since it's no longer necessary.
- Jun Rao
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
>
>
