Vihang Karajgaonkar 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: (13 comments) Took a first pass and left some comments and suggestions. Thanks a lot for fixing this. http://gerrit.cloudera.org:8080/#/c/15068/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15068/7//COMMIT_MSG@20 PS7, Line 20: String.intern() Curisous to know if we have any references like past JIRAs to show to String interning causes problems? In my experience, since Java 8 String.intern() is pretty stable and useful on large scale systems (eg: HMS uses it for interning the partition descriptions etc after object deserialization) http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java: http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@49 PS7, Line 49: public class SentryAuthorizationPolicy implements FilteredPrivilegeCache { Do you think we should refactor this code such that we can revert back to PrivilegeCache if needed? One way to do that would be to extend this class which implements FilteredPrivilegeCache and override the methods as needed. Then we can use some configuration to instantiate the right class and we can easily revert to older behavior. That would also mean that the changes in the PrincipalCache will use a NoOpPrivilegeTree instance in case the config is turned off. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@156 PS7, Line 156: filter It unclear why we are not storing the uri here? Can you clarify? 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@118 PS7, Line 118: Set<String> results = new HashSet<>(); May be I am overthinking this. Is there a race here? We are releasing the readlock while still accessing the PrincipalPrivilege's fields. In order to truely guarantee that this will not have races, we would need PrincipalPrivilege to be Immutable object but its not. I don't see a problem currently with this implementation but we should atleast add a comment here explaining why releasing the readlock is safe. 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: Tree that allows efficient lookup Can we have a more detailed documentation of the class? Specifically, why is it more efficient and if there are any caveats? http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@41 PS7, Line 41: Node<PrincipalPrivilege> tableRoot_ = new Node<>(); : : // Contains URI privileges grouped by server name. : Node<PrincipalPrivilege> uriRoot_ = new Node<>(); : : // Contains server privileges grouped by server name. : Node<PrincipalPrivilege> serverRoot_ = new Node<>(); Can you clarify why we need to have 3 separate trees instead of just one? http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@105 PS7, Line 105: Lossy representation Unclear what this means. Can you please clarify? http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@124 PS7, Line 124: server_ Does server double up for storing uris too? http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@138 PS7, Line 138: Only used to store privileges, but creating a generic class seemed clearer. probably unnecessary and can be removed. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@140 PS7, Line 140: private static class Node<T> { nit, can we move this class near the top? http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150 PS7, Line 150: depth +1 to Daniel's first comment above. Since all the public usage of this method is always with depth 0 perhaps make this method private and add a public method which takes in just the value and path. Same for the remove method as well. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@152 PS7, Line 152: if (values_ == null) values_ = new HashSet<>(); Do we need a sanity check here? Preconditions.checkState(path.size() == depth-1)? Can skip the Precondititions check if we mark this method as private so that we always control the value of depth. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@172 PS7, Line 172: if (values_.isEmpty()) values_ = null; Will this line ever get executed given the Preconditions check above? -- 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 <[email protected]> Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 05 Feb 2020 20:07:19 +0000 Gerrit-HasComments: Yes
