Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12020 )
Change subject: IMPALA-7917 (Part 1): Decouple Sentry from Impala ...................................................................... Patch Set 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java File fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java: http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java@46 PS8, Line 46: } > I wonder, is this how we want to handle extensions? Enumerate the models, t This code will be removed in: https://issues.apache.org/jira/browse/IMPALA-7918 http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@164 PS8, Line 164: break; > Nit: Would it make sense to have a method analyzer.authRequetBuilder() that Actually I can simplify that to analyzer.getServerName() instead. I also renamed toRequest() to build() since it's a more common name for a builder. http://gerrit.cloudera.org:8080/#/c/12020/8/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/12020/8/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@32 PS8, Line 32: AuthorizationProvider getProvider(); > Nit: getName() would suggest that this returns a String. Threw me off when Ah yeah, it was a string but I decided to changed it to enum instead and forgot to update this. Good catch! Done. http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableDb.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableDb.java: http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableDb.java@30 PS8, Line 30: public class SentryAuthorizableDb extends SentryAuthorizable { > Here and others. The trick in adapters such as this is defining the semanti I did a prototype using this design with Ranger and it works pretty well. In the next patchset, I updated the interface to remove methods that are too specific to Sentry. http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@285 PS8, Line 285: authConfig.getProvider() == AuthorizationProvider.SENTRY) { > In the final code, we're probably not going to want provider-specific code Yeah that's also my goal. One thing for sure with Sentry, Impala handles the caching layer. Ranger already has a built-in caching, in other words, there's no need to have RangerProxy (that also means we don't need to authorization catalog). We may have to clean up some of the authorization catalog as well. http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@289 PS8, Line 289: sentryProxy_ = null; > Now that you've defined a proxy class, is it simpler to define a "Null" pro I plan to do more refactoring on this proxy class since with Ranger, I don't think we need it. It'll probably in the second part of this patch though. Good suggestion on using "null/dummy" provider though. http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/12020/8/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@267 PS8, Line 267: private final AuthorizationPolicy authPolicy_ = new SentryAuthorizationPolicy(); > Another good place for a factory to avoid need for this code to be aware of Yeah, in the second part of this patch, I plan to create a factory and also add an additional flag in impala to specify which authorization provider to use. http://gerrit.cloudera.org:8080/#/c/12020/8/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/12020/8/fe/src/main/java/org/apache/impala/service/Frontend.java@257 PS8, Line 257: > Another place where differences can be abstracted out so we don't have prov This code will be removed in https://issues.apache.org/jira/browse/IMPALA-7918. It's very Sentry specific and has been deprecated. http://gerrit.cloudera.org:8080/#/c/12020/8/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/12020/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java@120 PS8, Line 120: new SentryAuthorizationConfig(sentryConfig), getServiceId(), cfg.principal, > Abstract out into a factory class? Same comment about factory class. Will do this in the second part of the patch. http://gerrit.cloudera.org:8080/#/c/12020/8/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/12020/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@142 PS8, Line 142: SentryAuthorizationConfig authConfig = new SentryAuthorizationConfig(cfg.server_name, > More implementation detail leakage? Same comment about this. Will be fixed in the second part of the patch. -- To view, visit http://gerrit.cloudera.org:8080/12020 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1fd1df0b38ddd7cfa41299e95f5827f8a9e9c1f Gerrit-Change-Number: 12020 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 14 Feb 2019 01:07:58 +0000 Gerrit-HasComments: Yes
