> On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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.
Thank you for the review! I am going through the comments and prepare a second patch. I did not know about this Java8 discussion, I just checked the pom.xml. Should I change the maven.compile.source to 1.7? > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java > > Lines 26 (patched) > > <https://reviews.apache.org/r/63415/diff/1/?file=1872671#file1872671line26> > > > > 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. I am not sure how KeyValue is useful here, but as I see, the API did actually parsing. Nothing used this API, only the string API and KeyValue was the parser. So extracting that would mean we need a parser as well, that would do the same thing as public KeyValue(String keyValue) does. I would rather move the content of KeyValue(String keyValue) to AuthorizableFactory.create(String s), so the string parsing could be shared among the all the AuthorizableFactories (and still allow custom parsing). > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1872672#file1872672line25> > > > > Since create(type, name) can be static, you don't really need an > > instance object I am sorry, but how could create(..) be static? It is part of the interface. > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1872672#file1872672line27> > > > > Please mark this as @Override But this method does not override anything. That would not compile. > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1872672#file1872672line38> > > > > This one throws COnfigurationException while the original interface > > didn't >From is the legacy static API and it did have that. > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1872672#file1872672line54> > > > > Please use a different name here - use of overloading here is confusing. > > > > Also, shouldn't this be a static method? The reason behind having non-static implementation, so these factory methods can be called in an abstraction. If you take a look at the GenericPrivilegeConverter, you may see that currently it has "if"s for every class. As static cannot be generalized with interface, the actual implementation need to be moved to implementation of a specific interface. I am using the name "create" instead of "from" to keep the original API. If you suggest to remove the static API altogether, I can remove the static calls from the PrivilegeValidators and tests. > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1872675#file1872675line151> > > > > 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. Adding doc for privilegeStrParser member: Optional function to parse or convert privilege string. BTW, only Kafka needed such, that is why there was an if-kafka block. > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1872675#file1872675line176> > > > > 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 Removed exception and changed back to private. > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1872675#file1872675line193> > > > > This seems to be unused - why is it defined? Left for extensibility. > On Oct. 30, 2017, 10:40 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1872675#file1872675line198> > > > > Is there any reason not to set it in constructor? This is optional. Only Kafka sets it. - Mano ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63415/#review189652 ----------------------------------------------------------- On Oct. 31, 2017, 2:20 p.m., Mano Kovacs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63415/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2017, 2:20 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/AbstractAuthorizableFactory.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableType.java > PRE-CREATION > > sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaAuthorizable.java > 52ae614b7f44fc351406f5b4ca6d2a631b8ec750 > > 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/SearchModelAuthorizable.java > 5a55963d40a9ad959660cf5381d3c2c6269ab0db > > 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/SqoopAuthorizable.java > 934875efbeff9ee66575e6e2a71c069da6e56cc4 > > 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/2/ > > > Testing > ------- > > Unit-tests > > > Thanks, > > Mano Kovacs > >
