Fredy Wijaya 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 8: (5 comments) 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: privileges.addAll(role.getPrivilegeNames()); : } : } > I think that this could be further simplified to privileges.addAll(role.get Done http://gerrit.cloudera.org:8080/#/c/13095/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@84 PS7, Line 84: } : return privileges; : } > Same as line 66. Done 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) { : return principalPrivileges_.add(privilege); > > > if non-URI parts of the name are always lower case, then it I decided to change the whole thing to make the cache stores the privilege name in a case sensitive way as per your original suggestion and I do agree that this is actually a better solution. Interestingly I found various bugs while fixing this, which will be updated in the next patchset. 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? principalPr Done. http://gerrit.cloudera.org:8080/#/c/13095/2/fe/src/main/java/org/apache/impala/catalog/Principal.java@97 PS2, Line 97: return principalPrivileges_.remove(privilegeName); : } > Same as above. Done -- 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: 8 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 20:28:11 +0000 Gerrit-HasComments: Yes
