----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42568/#review116297 -----------------------------------------------------------
sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Host.java (line 20) <https://reviews.apache.org/r/42568/#comment177329> Are these object names expected to be case insensitive? Should comment on that someplace. sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionConstant.java (line 22) <https://reviews.apache.org/r/42568/#comment177337> Do you support "ALL_NAME"? sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionFactory.java (line 24) <https://reviews.apache.org/r/42568/#comment177334> Seems like this should be a singleton class (or even better, just have static methods to create actions - but that might require some refactoring). sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionFactory.java (line 27) <https://reviews.apache.org/r/42568/#comment177336> Would it be better to just do something like: READ(new KafkaAction(KafkaActionConstant.READ, 1)); Then you don't have to create a new instance of KafkaAction and match the names with each call. sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionFactory.java (line 59) <https://reviews.apache.org/r/42568/#comment177346> Return null instead, and comment on return val? sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionFactory.java (line 71) <https://reviews.apache.org/r/42568/#comment177345> Should this really throw here or just return null/empty list? sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionFactory.java (line 98) <https://reviews.apache.org/r/42568/#comment177339> clarify sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaAuthorizable.java (line 22) <https://reviews.apache.org/r/42568/#comment177326> Can you expand the comment to explain what each of the authorizable objects map to (within Kafka) sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAction.java (line 28) <https://reviews.apache.org/r/42568/#comment177332> Brief comment on goal of the test suite sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAction.java (line 128) <https://reviews.apache.org/r/42568/#comment177343> Negative tests for invalid actions sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAuthorizable.java (line 29) <https://reviews.apache.org/r/42568/#comment177331> Brief comment on the goal of the test suite - Lenni Kuff On Jan. 21, 2016, 10:29 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42568/ > ----------------------------------------------------------- > > (Updated Jan. 21, 2016, 10:29 p.m.) > > > Review request for sentry and Dapeng Sun. > > > Bugs: SENTRY-1012 > https://issues.apache.org/jira/browse/SENTRY-1012 > > > Repository: sentry > > > Description > ------- > > SENTRY-1012: Add core model for Kafka > > > Diffs > ----- > > pom.xml 6210454e87dc1fd3e6fc4abe10e42cf7b02591aa > sentry-core/pom.xml 59d32c4da0a6de77510d142efd006c345628c4a5 > sentry-core/sentry-core-model-kafka/pom.xml PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Cluster.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/ConsumerGroup.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Host.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionConstant.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionFactory.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaAuthorizable.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Topic.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAction.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAuthorizable.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/42568/diff/ > > > Testing > ------- > > Added unit tests. Also covered via end-to-end tests as part of SENTRY-1014. > > > Thanks, > > Ashish Singh > >
