----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42778/#review116273 -----------------------------------------------------------
Thank Ashish, Here are my comments. sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaModelAuthorizables.java (line 51) <https://reviews.apache.org/r/42778/#comment177292> Fix indentation sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaWildcardPrivilege.java (line 62) <https://reviews.apache.org/r/42778/#comment177305> We need to separate this part logic from Kafka, Sqoop and Solr... and add them to a common abstract privilege. I think we should file a jira at Sentry to refine it. sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/ResourceRequiredMatch.java (line 35) <https://reviews.apache.org/r/42778/#comment177299> Nit: You can also change it to KafkaPrivilegeValidator sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/ResourceRequiredMatch.java (line 45) <https://reviews.apache.org/r/42778/#comment177296> Since only hostname is required for a privilege, please consider to remove resourceMatched and the check of other resources. sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/ResourceRequiredMatch.java (line 49) <https://reviews.apache.org/r/42778/#comment177297> Since only hostname is required for a privilege, please consider to remove resourceMatched and the check of other resources. sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/ResourceRequiredMatch.java (line 53) <https://reviews.apache.org/r/42778/#comment177298> Since only hostname is required for a privilege, please consider to remove resourceMatched and the check of other resources. sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestResourceNameRequiredMatch.java (line 27) <https://reviews.apache.org/r/42778/#comment177302> Rename the className and methodNames sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestResourceNameRequiredMatch.java (line 28) <https://reviews.apache.org/r/42778/#comment177303> Please add some UTs for unexpected privilege, for example: hosst, acion, conne... sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaPolicyNegative.java (line 41) <https://reviews.apache.org/r/42778/#comment177293> Could be removed? - Dapeng Sun On 一月 26, 2016, 12:40 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42778/ > ----------------------------------------------------------- > > (Updated 一月 26, 2016, 12:40 p.m.) > > > Review request for sentry and Dapeng Sun. > > > Bugs: SENTRY-1013 > https://issues.apache.org/jira/browse/SENTRY-1013 > > > Repository: sentry > > > Description > ------- > > SENTRY-1013: Add policy engine for Kafka > > > Diffs > ----- > > pom.xml aa4a021eae93c3c817ec055e18868b95f7cba124 > sentry-policy/pom.xml ef938a6b52bb00042ea8ae32da3d245ef0697a00 > sentry-policy/sentry-policy-kafka/pom.xml PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaModelAuthorizables.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaWildcardPrivilege.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/ResourceRequiredMatch.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/SimpleKafkaPolicyEngine.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/KafkaPolicyFileProviderBackend.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/MockGroupMappingServiceProvider.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestKafkaModelAuthorizables.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestKafkaWildcardPrivilege.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestResourceNameRequiredMatch.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/engine/AbstractTestKafkaPolicyEngine.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/engine/TestKafkaPolicyEngineDFS.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/engine/TestKafkaPolicyEngineLocalFS.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaAuthorizationProviderGeneralCases.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaAuthorizationProviderSpecialCases.java > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaPolicyNegative.java > PRE-CREATION > sentry-policy/sentry-policy-kafka/src/test/resources/log4j.properties > PRE-CREATION > > sentry-policy/sentry-policy-kafka/src/test/resources/test-authz-provider.ini > PRE-CREATION > > Diff: https://reviews.apache.org/r/42778/diff/ > > > Testing > ------- > > Tested via unit tests and end-to-end tests that will be added as part of > SENTRY-1014. > > > Thanks, > > Ashish Singh > >
