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 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationProxy.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationProxy.java: http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationProxy.java@222 PS5, Line 222: m authoriz > nit: should be lower case Done http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/Privilege.java File fe/src/main/java/org/apache/impala/authorization/Privilege.java: http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/Privilege.java@23 PS5, Line 23: * List of Impala privileges. > not understanding this Done http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/Privilege.java@40 PS5, Line 40: static { > it's sort of odd that you use a static block to set up the 'privileges_' me Java doesn't allow this construct. public enum Privilege { ... VIEW_METADATA(EnumSet.of(INSERT, SELECT, REFRESH), true); // compilation error. Privilege(EnumSet<Privilege> privileges, boolean anyOf) { ... } } http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/Privilege.java@53 PS5, Line 53: Privilege> implied_; > maybe this should be like 'implied_' or something? If I understand correctl Yeah implied is a better name. It basically means if a SQL has VIEW_METADATA privilege, it requires either INSERT, SELECT, or REFRESH. http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequest.java File fe/src/main/java/org/apache/impala/authorization/PrivilegeRequest.java: http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequest.java@45 PS5, Line 45: > seems to no longer be permitted Done http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java File fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java: http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java@74 PS5, Line 74: /** > worth adding Preconditions.checkState(authorizable_ == null) in all of thes Done http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaAction.java File fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaAction.java: http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaAction.java@48 PS5, Line 48: mpalaAction(String value, int code) { : bitFieldAction_ = new BitFieldAction(value, code); : } > do we still need to support both versions? maybe we can have a TODO to remo We only need to support Sentry 2.0.0. I'll update the code. Done. -- 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: 7 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> 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: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 07 Jan 2019 17:34:41 +0000 Gerrit-HasComments: Yes
