----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63415/#review190501 -----------------------------------------------------------
Overall looks good, some style nits/comments. Colm is also poking around this area - can you ask him to check this as well? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AbstractAuthorizableFactory.java Lines 27 (patched) <https://reviews.apache.org/r/63415/#comment267927> missorted modifiers - this should be public abstract sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AbstractAuthorizableFactory.java Lines 30 (patched) <https://reviews.apache.org/r/63415/#comment267932> Essentially you are providing a helper method that converts string to a key/value pair. It is of course possible doing it via intermediate abstract class, but I would consider providing a static helper method instead. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AbstractAuthorizableFactory.java Lines 41 (patched) <https://reviews.apache.org/r/63415/#comment267933> I would consider using preconditions instead: ``` type = kvList.get(0); Preconditions.checkArgument(!type.isEmpty(), "Type cannot be empty"); name = kvList.get(1); Preconditions.checkArgument(!name.isEmpty(), "Name cannot be empty"); ``` sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AbstractAuthorizableFactory.java Lines 42 (patched) <https://reviews.apache.org/r/63415/#comment267958> Where does the input string come from? Does it come from a user input or from programmatic strings? If it comes from programmatic input, throwing IllegalArghumentException is valid, but if it comes from user, I think this should be a checked exception. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AbstractAuthorizableFactory.java Lines 49 (patched) <https://reviews.apache.org/r/63415/#comment267934> Why are you returning IllegalArgumentException in other cases but do swallow it here? Other create() methods don't do it. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AbstractAuthorizableFactory.java Lines 58 (patched) <https://reviews.apache.org/r/63415/#comment267928> IllegalArgumentException is unchecked RuntimeException so the throws part isn't needed. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AbstractAuthorizableFactory.java Lines 67 (patched) <https://reviews.apache.org/r/63415/#comment267938> If you restrict T to Enum<E> you can use values() method of enums instead. Also you have missorted modifiers - it should be protected abstract sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java Lines 24 (patched) <https://reviews.apache.org/r/63415/#comment267955> Please document whether these create() methods can return null and if so in such cases. I would argue that they shouldn't, but it is up to you to define such contract. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableType.java Lines 19 (patched) <https://reviews.apache.org/r/63415/#comment267930> Please document this interface. This looks a bit strange - why would something which has a single interface called name() will be called 'AuthorizableType'? Why is this parameterized by type T which isn't used? Looks like you can drop the T completely. Looks like this is always used for enums so I would suspect that you are trying to reinvent the enum valueOf() mechanism here. Can you take another look at this and see whether this part can be simplified by using Enum.valueOf() instead? sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java Line 24 (original), 23 (patched) <https://reviews.apache.org/r/63415/#comment267942> instance can be final sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchModelAuthorizables.java Lines 22 (patched) <https://reviews.apache.org/r/63415/#comment267944> should be `private static final` sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Lines 37 (patched) <https://reviews.apache.org/r/63415/#comment267949> This is pre-existing code, but I'd consider using ArrayList instead of LinkedList sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Lines 153 (patched) <https://reviews.apache.org/r/63415/#comment267959> Looks like this method is never used sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 173 (original), 157 (patched) <https://reviews.apache.org/r/63415/#comment267960> Looks like this method is never used sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 182 (original), 161 (patched) <https://reviews.apache.org/r/63415/#comment267961> can this be package-private? - Alexander Kolbasov 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 > >
