----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63415/#review189652 -----------------------------------------------------------
This isn't a full review - just looked at some of the changes - probably comments apply in other files as well. The biggest concern - please stay away from Java8-specific features. Although there is decision to move to Java8 for compilation, there is no consensus on when should Java8 specific changes be allowed. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java Lines 21 (patched) <https://reviews.apache.org/r/63415/#comment266842> Please provide documentation for this class - it is pretty basioc, so should have very good documentation. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java Lines 26 (patched) <https://reviews.apache.org/r/63415/#comment266844> I think KeyValue is a useless class which shouldn't be used by more things. We should either use Pair or something specifically useful for Sentry. Part of that, we shouldn't include enforce parsing in the API, so the API should get both key and values as separate entities. sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java Lines 25 (patched) <https://reviews.apache.org/r/63415/#comment266858> Since create(type, name) can be static, you don't really need an instance object sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java Line 24 (original), 27 (patched) <https://reviews.apache.org/r/63415/#comment266854> Please mark this as @Override sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java Line 35 (original), 38 (patched) <https://reviews.apache.org/r/63415/#comment266855> This one throws COnfigurationException while the original interface didn't sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java Line 39 (original), 54 (patched) <https://reviews.apache.org/r/63415/#comment266857> Please use a different name here - use of overloading here is confusing. Also, shouldn't this be a static method? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Lines 57 (patched) <https://reviews.apache.org/r/63415/#comment266851> This is Java-8 only feature - we do not have consensus on using any Java8 features in Sentry code. FOr now please stick to Java 7 features. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 116 (original), 110 (patched) <https://reviews.apache.org/r/63415/#comment266845> Since you are changing the code, can you also convert it to foreach construct? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 138 (original), 132 (patched) <https://reviews.apache.org/r/63415/#comment266850> Do we have a case of parser that is not set? What does it mean to "parse" a privilege string - please add doc explaining what this function is supposed to do. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 161 (original), 150 (patched) <https://reviews.apache.org/r/63415/#comment266846> Why does it throws an exception? Do you need it - looks like it is only used here - why not use privilegeValidators directly? If you want to make it protected, please add documentation sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 173 (original), 158 (patched) <https://reviews.apache.org/r/63415/#comment266847> Why do you need a one-liner function here - can you just inline the call? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 177 (original), 162 (patched) <https://reviews.apache.org/r/63415/#comment266849> This seems to be unused - why is it defined? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 182 (original), 166 (patched) <https://reviews.apache.org/r/63415/#comment266848> Is there any reason not to set it in constructor? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java Lines 40 (patched) <https://reviews.apache.org/r/63415/#comment266852> Please do not use lambdas for Sentry code - currently it should work with Java 7. - Alexander Kolbasov On Oct. 30, 2017, 3:42 p.m., Mano Kovacs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63415/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2017, 3:42 p.m.) > > > Review request for sentry and Colm O hEigeartaigh. > > > Bugs: SENTRY-2012 > https://issues.apache.org/jira/browse/SENTRY-2012 > > > Repository: sentry > > > Description > ------- > > The patch makes SentryShellGeneric and GenericPrivilegeConverter independent > of any component. > > > Diffs > ----- > > bin/sentryShell 17b1429f096bc70c2dead8dbf708fd039baf319d > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java > 45a1148533ec4eb0f506b2dc6c81df6cc1663870 > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchModelAuthorizables.java > 2b190e5ca2b49ca5737a9835e4b301317138ac21 > > sentry-core/sentry-core-model-sqoop/src/main/java/org/apache/sentry/core/model/sqoop/SqoopModelAuthorizables.java > 3bb9a19a36f297c8f6c94c0f6d112873ba2e32fb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java > 51d6df958aea7681c62a751f25e3d2d624dc96dd > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java > 77d39194f548dad8700d6f6ef6330c2fa0b92579 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellGeneric.java > 49f18c8994df27217592fafc04ca3b96eaa4a978 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSolr.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSqoop.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryConfigToolSolr.java > 3685073910e4b5f45183742723c6146749a927f1 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellKafka.java > 80bbcf184084a0a558351f2872ca0bb922c81aa3 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java > 55831a45b4e75fefecbebd8ce13da73ce3697d4f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSqoop.java > 7bafd8c40c0c0be2b91fac4932bdb15709a379fd > > > Diff: https://reviews.apache.org/r/63415/diff/1/ > > > Testing > ------- > > Unit-tests > > > Thanks, > > Mano Kovacs > >
