----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43977/#review121214 -----------------------------------------------------------
Thank Ashish, I left some comments. sentry-tests/sentry-tests-kafka/pom.xml (line 32) <https://reviews.apache.org/r/43977/#comment182884> The scope of some dependences should be "test" sentry-tests/sentry-tests-kafka/pom.xml (line 40) <https://reviews.apache.org/r/43977/#comment182881> Move the version to the top-level pom.xml sentry-tests/sentry-tests-kafka/pom.xml (line 49) <https://reviews.apache.org/r/43977/#comment182882> Move the version to the top-level pom.xml sentry-tests/sentry-tests-kafka/pom.xml (line 54) <https://reviews.apache.org/r/43977/#comment182883> Move the version to the top-level pom.xml sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 56) <https://reviews.apache.org/r/43977/#comment182889> need to check if kafkaServer is null sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 61) <https://reviews.apache.org/r/43977/#comment182890> need to check if zkServer is null sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 68) <https://reviews.apache.org/r/43977/#comment182886> need sync? sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 69) <https://reviews.apache.org/r/43977/#comment182887> zkServerSocket should be renamed sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 73) <https://reviews.apache.org/r/43977/#comment182888> Should return the exception, otherwise there will be a NullPointerException sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 83) <https://reviews.apache.org/r/43977/#comment182891> Throw a runtime exception here? sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 109) <https://reviews.apache.org/r/43977/#comment182885> CumtomePrincipalBuilder is unused? should we delete it? sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 114) <https://reviews.apache.org/r/43977/#comment182893> How about change generate to setup? sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java (line 123) <https://reviews.apache.org/r/43977/#comment182892> Throw exception? sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/TestUtils.java (line 21) <https://reviews.apache.org/r/43977/#comment182894> Can we use FileUtils at apache common? sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/TestUtils.java (line 21) <https://reviews.apache.org/r/43977/#comment182895> Can we use FileUtils at apache common? sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java (line 18) <https://reviews.apache.org/r/43977/#comment182896> The comments need to move before the class name sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java (line 59) <https://reviews.apache.org/r/43977/#comment182897> Is **getHostname()** okay? and it should not use port 80. sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAclsCrud.java (line 44) <https://reviews.apache.org/r/43977/#comment182901> Is sentryKafkaAuthorizer depended on AbstractKafkaSentryTestBase? If not, we should move the tests to "SENTRY-1057 Add implementations for acls' CRUD" sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java (line 52) <https://reviews.apache.org/r/43977/#comment182900> The indentation should be two Spaces - Dapeng Sun On 二月 25, 2016, 9:53 a.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43977/ > ----------------------------------------------------------- > > (Updated 二月 25, 2016, 9:53 a.m.) > > > Review request for sentry, Dapeng Sun and Hao Hao. > > > Bugs: SENTRY-1014 > https://issues.apache.org/jira/browse/SENTRY-1014 > > > Repository: sentry > > > Description > ------- > > SENTRY-1014: Add E2E tests for Kafka plugin. > > > Diffs > ----- > > pom.xml ca2c92a26e3e42fe036c974235db47255d1465de > sentry-binding/sentry-binding-kafka/pom.xml > bd24c20edcbffb5caee69aedddb93cb5a6c8c112 > > sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizer.java > 9ffb971d8da51b716ece3ff5a5bbeebcc7f74e8d > > sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java > 9e72d7890e0db3710ab0971ccbb71a32d3d36e87 > > sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBindingSingleton.java > 92e50e6452a89e676eb559a3657b17ba563bfddc > > sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/conf/KafkaAuthConf.java > e75ec7eddaa1a48b731d551736859a94c08af593 > > sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizerTest.java > eafe0f0ee5482fadee43ddec99bdc5da3f42e30f > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Cluster.java > bb30b1b78d4dccaf8ea5fe22eeed496b608cfcb1 > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionFactory.java > 7b8b5187e3d95a49304512e238e778da885fd27d > > sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAuthorizable.java > 20d5e8e518cb91b32fad930e9d39400764f40b55 > > sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaModelAuthorizables.java > f1ed0001883e5d4b362389c258f4f72697a1e5a4 > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestKafkaModelAuthorizables.java > 513c2719424f994e8eb773e72073401b465b7d58 > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaAuthorizationProviderGeneralCases.java > bcc119860860d928d092ed3ddc0a5a28e841c9aa > sentry-provider/sentry-provider-db/pom.xml > 7514a7cdfcc7934f2dd0386996fdaf88c0ccbb14 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java > c3b0be8694c746cb09797425f98578b8faef8b4a > sentry-tests/pom.xml 3294335e95fb7dbb2da4151041269392004426fb > sentry-tests/sentry-tests-kafka/pom.xml PRE-CREATION > > sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/CustomPrincipalBuilder.java > PRE-CREATION > > sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/EmbeddedZkServer.java > PRE-CREATION > > sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java > PRE-CREATION > > sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/TestUtils.java > PRE-CREATION > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java > PRE-CREATION > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/StaticUserGroupRole.java > PRE-CREATION > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAclsCrud.java > PRE-CREATION > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java > PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/log4j.properties > PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/test.crt PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/test.keystore.jks > PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/test.truststore.jks > PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/user1.crt PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/user1.keystore.jks > PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/user1.truststore.jks > PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/user2.crt PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/user2.keystore.jks > PRE-CREATION > sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks > PRE-CREATION > > Diff: https://reviews.apache.org/r/43977/diff/ > > > Testing > ------- > > Added E2E tests. > > Note that this contains changes from SENTRY-1098, SENTRY-1056, SENTRY-1030 > and SENTRY-1057, and will have to be rebased once they get it. > > > Thanks, > > Ashish Singh > >
