Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala ...................................................................... Patch Set 3: (7 comments) First-pass review. Sorry, was started on PS 2; I see you're now on PS 3. http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java: http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java@46 PS2, Line 46: case SENTRY: I wonder if there is another way to do this? This class would appear to need to enumerate all the auth providers. As such, it breaks abstraction. Can these methods be defined within each auth plugin itself? Sentry, say, knows to create its own authorizables? http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java: http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java@26 PS2, Line 26: public class AuthorizableTable extends Authorizable { Looks like there are a series of these classes. Each takes a series of names. I wonder, does this generalize? Or, should these be built via composition? An auth table takes an auth DB and a table name? An auth column takes an auth table and a column name? I'm not proposing we make the change if it's not needed, just trying to reason out the data model. http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java: http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@60 PS2, Line 60: static SentryAuthorizationConfig sentry(String serverName, Similar to prior comments: should the sentry-specific binding be in the generic interface, or should this reside in some sentry-specific driver? http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java: http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java@499 PS2, Line 499: Set<String> groupNames = ((SentryAuthorizationChecker) fe.getAuthzChecker()) Temporary code? Or, should this be handled by sentry-specific code? http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/Frontend.java@321 PS2, Line 321: if (authzConfig_.getProvider() == AuthorizationProvider.SENTRY) { Similar to earlier: should Sentry-specific stuff leak out of the abstraction, or should this stuff be handled through a generic interface? http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java@111 PS2, Line 111: if (BackendConfig.INSTANCE.useSentry()) { Should this be factored to into a an auth factory rather than inline here in this top-level class? http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@143 PS2, Line 143: if (BackendConfig.INSTANCE.useSentry()) { Ditto -- To view, visit http://gerrit.cloudera.org:8080/12542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86 Gerrit-Change-Number: 12542 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Austin Nobis (450) Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Comment-Date: Sat, 23 Feb 2019 04:38:56 +0000 Gerrit-HasComments: Yes
