Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12020 )
Change subject: IMPALA-7917 (Part 1): Decouple Sentry from Impala ...................................................................... Patch Set 6: (7 comments) I just realized I had some pending comments on an old rev of this patch. I didn't get all the way through, but will publish these before I take another look (hopefully next week) 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 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: * Maps an Impala privilege to one or more Impala privileges. not understanding this 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_' member, but a constructor parameter to set up the 'anyOf_' member http://gerrit.cloudera.org:8080/#/c/12020/5/fe/src/main/java/org/apache/impala/authorization/Privilege.java@53 PS5, Line 53: Privilege> privileges_ maybe this should be like 'implied_' or something? If I understand correctly, this is saying that VIEW_METADATA privilege implies INSERT, SELECT, and REFRESH? (not sure I understand, semantically, why that's the case though) 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: Authorizable refers to the server if it's null seems to no longer be permitted 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: public PrivilegeRequestBuilder onTable(String dbName, String tableName) { worth adding Preconditions.checkState(authorizable_ == null) in all of these functions to prevent someone from calling conflicting builder setters? 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: / In Sentry 1.5.1, BitFieldAction is an abstract class. In Sentry 2.0.0, : // BitFieldAction is no longer an absract class. To support both versions, : // we can extend BitFieldAction. do we still need to support both versions? maybe we can have a TODO to remove this? at some point down the line? -- 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: 6 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: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Sat, 05 Jan 2019 00:14:10 +0000 Gerrit-HasComments: Yes
