Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13095 )
Change subject: IMPALA-8444: Fix performance regression when building privilege name ...................................................................... Patch Set 7: (4 comments) I am still uncertain about the case sensitivity, once that is clarified, I can give +2. http://gerrit.cloudera.org:8080/#/c/13095/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/13095/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@66 PS7, Line 66: for (String privilegeName: role.getPrivilegeNames()) { : privileges.add(privilegeName); : } I think that this could be further simplified to privileges.addAll(role.getPrivilegeNames()); It is also possible that the union is a bit faster than inserting one-by-one, especially when adding the first set to the initial empty set. http://gerrit.cloudera.org:8080/#/c/13095/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@84 PS7, Line 84: for (String privilegeName: user.getPrivilegeNames()) { : privileges.add(privilegeName); : } Same as line 66. http://gerrit.cloudera.org:8080/#/c/13095/2/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/13095/2/fe/src/main/java/org/apache/impala/catalog/Principal.java@63 PS2, Line 63: */ : public boolean addPrivilege(PrincipalPrivilege privilege) { : // URIs are case sensitive. Sentry treats everything in the URI > > if non-URI parts of the name are always lower case, then it > doesn't matter if we treat them as case insensitive or not > > When calling getPrivilege(String privilegeName) for non URI, we > need to know the exact case that's stored in the cache. It's > simpler to special case the URI and make everything else to be case > insensitive. > This is still not clear to me. Keys for non-URIs are lowered twice: once in buildPrivilegeName() (or action and grant option can contain upper case latters?), and once again in CatalogObjectCache functions. If the goal is to find non-URI privileges in getPrivilege() even if there are upper/lower case differences, then this could be achieved by adding function to PrincipalPrivilege like string convertToCacheKey(string privilegeName) that check if the name is an URI, and call toLower() if it is not. I would prefer this because it would move the case handling issues to a single class (that contains buildPrivilegeName()). + see my comment for getPrivilege() > > if non-URI parts of the name can be upper case (including > 'server'), then this add() will also treat the 'server' part of > URIs in case sensitive way, which seems wrong > > Yeah I agree, this is weird. This is actually a bug in Sentry. > Sentry treats everything in the URI as case sensitive including the > host name. We can fix it by always lower casing the host name > before sending it to Sentry. However if a privilege was granted > outside Impala that contains upper case host name, there's nothing > much that Impala can do. I'll add a comment in the code. Ouch, thanks for the explanation. http://gerrit.cloudera.org:8080/#/c/13095/2/fe/src/main/java/org/apache/impala/catalog/Principal.java@89 PS2, Line 89: Don't we need to handle URIs case sensitively in this function? principalPrivileges_.get(privilegeName) will call toLower() on privilegeName, so it will not match with URI keys if they contain upper case latter. -- To view, visit http://gerrit.cloudera.org:8080/13095 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I942d9b55f07c8972f69e532567d9b7d80fceb6e5 Gerrit-Change-Number: 13095 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 25 Apr 2019 15:40:58 +0000 Gerrit-HasComments: Yes
