You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org> on 2019/03/04 21:48:41 UTC
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 7:
(9 comments)
Nice clean separation. The patch looks pretty good to me. Some minor comments, would like to know your thoughts on them.
http://gerrit.cloudera.org:8080/#/c/12542/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12542/7//COMMIT_MSG@15
PS7, Line 15: ipmlementation
nit: typo
http://gerrit.cloudera.org:8080/#/c/12542/7/be/src/service/frontend.cc
File be/src/service/frontend.cc:
http://gerrit.cloudera.org:8080/#/c/12542/7/be/src/service/frontend.cc@43
PS7, Line 43: DEFINE_string(server_name, "", "The name to use for securing this impalad "
: "server during authorization. Set to enable authorization. By default, the "
: "authorization policy will be loaded from the catalog server (via the statestore)."
: "To use a file based authorization policy, set --authorization_policy_file.");
: DEFINE_string(authorization_policy_file, "", "HDFS path to the authorization policy "
: "file. If set, authorization will be enabled and the authorization policy will be "
: "read from a file.");
: DEFINE_string(authorization_po
Aren't these also Sentry specific? If so, do we need to call them out?
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@383
PS7, Line 383: AuthorizationConfig authzConfig
Do you need this too? should probably be inferred from the factory.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@29
PS7, Line 29: AuthorizationConfig newAuthorizationConfig(BackendConfig backendConfig);
Any reason to call them (this and below) getBlahBlah() instead of newBlahBlah()? For ex: getAuthzConfig() instead of newAuthorizationCofnig()?
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@40
PS7, Line 40: AuthorizationConfig authzConfig
Should this be in control of the AuthorizationFactoryClass? It looks like the current flow is like,
1. create AuthzConfig
2. createAuthznFactor(AuthzConfig).
The latter step could also build the AuthzConfig, no? It looks to me like the clients shouldn't worry about AuthznConfig anymore. They just pass the BackendConfig and totally rely on the created AuthznFactory from there on.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java
File fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java@25
PS7, Line 25: DefaultAuthorizableFactory
Whats the use of this class?
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java@28
PS7, Line 28: public
Make this private and use the interface method AuthorizationFactor#getAuthzConfig() at callers?
http://gerrit.cloudera.org:8080/#/c/12542/7/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/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java@116
PS7, Line 116: String.format("Unable to instantiate authorization provider: %s",
Log the error trace here?
http://gerrit.cloudera.org:8080/#/c/12542/7/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/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java@149
PS7, Line 149: String.format("Unable to instantiate authorization provider: %s",
nit: Can you also log the error here?
--
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: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Mar 2019 21:48:41 +0000
Gerrit-HasComments: Yes