Csaba Ringhofer 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 9: (17 comments) Thanks everyone for your comments! Some guide to view the new patch sets: PS8: adds unit tests and fixed a bug about not returning server privileges when listing URI privileges PS9: fixes update behavior - this also needed Nodes to store name->PrincipalPrivilege maps for values instead of sets of PrincipalPrivileges. PS10: Fixes most of the review 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 Done 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 r PrincipalPrivilege's fields are accessed at a lot of places in the code, so I am hesitant about adding a comment about this here. PrincipalPrivilege is practically treated as immutable - most of its fields are included in the name (returned by getName()), which is also used as a key in CatalogObjectCache, so changing any of these members would break our assumptions about CatalogObjectCache regardless of thread safety. The only way it should be changed is by replacing it with a new PrincipalPrivilege with the same name (and higher catalog version). If this happens on another thread, the old PrincipalPrivilege will be held by the current thread for the given privilege check - my understanding is that this is the intended way to use CatalogObjects. 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 i I added more details in the comment. 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? Done 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 I added unit tests which (of course :/) found some bugs (wrong update logic + not returning match server privileges when listing privileges for an URI). 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? Done http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@105 PS7, Line 105: return path; > Unclear what this means. Can you please clarify? Done http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@124 PS7, Line 124: > Does server double up for storing uris too? Yes, URIs are stored per server. Added some explanation in the class comment. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@125 PS7, Line 125: private List<String> toPath() { > if uri is not null, should you add it into path before return? The URI string are intentionally not added to the path. Added some explanation about this in the class comment. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@134 PS7, Line 134: > Nit: object. Done http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@138 PS7, Line 138: Tree node that holds the privileges for a given objects (server, database, > probably unnecessary and can be removed. I would prefer to leave it here - generally if there is a generic class, I would assume that it is expected to be reused with different classes. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@140 PS7, Line 140: * with paths like ["server1", "db1", "table1"]. > nit, can we move this class near the top? I would prefer to keep the members + public functions at the top and leave to more complex tree logic at the end. I am not sure if we have a general rule for this. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150 PS7, Line 150: /** > As an alternative, instead of passing 'depth', we could also pass a sublist I reversed that paths and removed the last element on every level at first - but this caused a bug as the same path is used with multiple "roots". http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150 PS7, Line 150: > +1 to Daniel's first comment above. Since all the public usage of this meth Done http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@167 PS7, Line 167: * Finds the Node at 'path' (it is treated as error if it doesn't exist), > See the comment about 'depth' for the 'add' method. Done http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@172 PS7, Line 172: if (path.size() <= depth) { > Will this line ever get executed given the Preconditions check above? Yes, it can be executed. "found" means that the given PrincipalPrivilege was really found and removed (the comment of the function mentions that it is an error of the element to remove doesn't exist), while the next line sets the value_ to null if it became empty to save some memory. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@200 PS7, Line 200: > See the comment about 'depth' for the 'add' method. Oops, I forgot this, I will do this in the next patch. -- 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: 9 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: Mon, 10 Feb 2020 15:48:15 +0000 Gerrit-HasComments: Yes
