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 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/13095/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13095/2//COMMIT_MSG@26 PS2, Line 26: : Benchmark Mode Cnt Score Error Uni > The numbers seem weird, as 10?³ == 0.001, so to only difference seems to be I used ms instead of us. I think for a number that's way too small, JMH doesn't show it correctly. I re-ran the benchmark with us instead. http://gerrit.cloudera.org:8080/#/c/13095/2//COMMIT_MSG@36 PS2, Line 36: URI privilege in a case sensitive manner. : : This patch removes the synchronized keyword in : SentryAuthorizationPolicy.listPrivileges() in order to not cause the : operation to run in seria > Note that I do not have a solid understanding about integration with Sentry As part of the refactoring AuthorizationPolicy to SentryAuthorizationPolicy, the concurrency model is no longer correct. In the past, it listPrivileges() used to be in AuthorizationPolicy. Here's what I mean by staleness. List<Role> grantedRoles = authzPolicy_.getGrantedRoles(groupName); // yes, this operation is thread-safe // by the time we reach here, another thread may be granting a new role into the group, hence we're now iterating on the stale granted roles for (Role role: grantedRoles) To avoid this issue, we can lock on authzPolicy_ in listPrivileges() but I also agree that this has a side effect of not allowing concurrent modifications. http://gerrit.cloudera.org:8080/#/c/13095/2/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/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@67 PS2, Line 67: if (privilegeName == null) { : if (LOG.isTraceEnabled()) { : LOG.trace("Ignoring invalid privilege: " + privilegeName); : } : continue; : } > I think that we do not need to handle the null case, as the keys cannot be I missed this. Done. http://gerrit.cloudera.org:8080/#/c/13095/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@91 PS2, Line 91: if (privilegeName == null) { : if (LOG.isTraceEnabled()) { : LOG.trace("Ignoring invalid privilege: " + privilegeName); : } : continue; : } > See comment at line 67. Done http://gerrit.cloudera.org:8080/#/c/13095/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java File fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java: http://gerrit.cloudera.org:8080/#/c/13095/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java@67 PS2, Line 67: * The case sensitivity of name will depend on the value of 'caseInsensitiveKeys'. > nit: missing 'of'? 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: // URIs are case sensitive. Sentry treats everything in the URI including the host : // name as case sensitive. : return principalPrivileges_.add(privilege, > 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. > 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. http://gerrit.cloudera.org:8080/#/c/13095/2/fe/src/main/java/org/apache/impala/catalog/Principal.java@97 PS2, Line 97: public PrincipalPrivilege removePrivilege(String privilegeName) { : // URIs are case sensitive. Sentry treats everything in the URI including the host > See comment at line 63. Same as above. http://gerrit.cloudera.org:8080/#/c/13095/2/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/13095/2/tests/authorization/test_grant_revoke.py@389 PS2, Line 389: test_uri_privilege_case_sensitive > Is it possible to add two uri privileges that only differ in case sensitivi 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: 3 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: Wed, 24 Apr 2019 16:40:23 +0000 Gerrit-HasComments: Yes
