Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java File fe/src/main/java/org/apache/impala/catalog/Principal.java: http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java@104 PS7, Line 104: * Returns all privilege names for this principal, or an empty set if no privileges are The doc comment is the same as for getPrivilegeNames() and does not mention the filter. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java: http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@34 PS7, Line 34: for Isn't this 'for' superfluous? http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@37 PS7, Line 37: public class PrincipalPrivilegeTree { The implementation seems correct to me, though I haven't tried it yet, only read it. I think it is a good idea to add unit tests. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@134 PS7, Line 134: s Nit: object. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150 PS7, Line 150: void add(T value, List<String> path, int depth) { I think 'depth' should be documented. Also it seems to me that depth is always 0 when the method is called from outside and any other depth value only appears during the recursion. I think it would be clearer to make a public wrapper method that calls this with depth=0 and make this method private. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@167 PS7, Line 167: void remove(T value, List<String> path, int depth) { See the comment about 'depth' for the 'add' method. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@200 PS7, Line 200: void getAllMatchingValues(List<T> results, List<String> path, int depth) { See the comment about 'depth' for the 'add' method. -- To view, visit http://gerrit.cloudera.org:8080/15068 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e Gerrit-Change-Number: 15068 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 04 Feb 2020 18:36:06 +0000 Gerrit-HasComments: Yes
