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 11: (4 comments) 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 Strin This change added a "handmade" interner to Impala: https://gerrit.cloudera.org/#/c/11158/ It mentions this post with Java 8 benchmarks: https://shipilev.net/jvm-anatomy-park/10-string-intern/ The intention of the change was to reduce memory consumption by interning some common strings, and it avoided String.intern() to be sure that no regression is introduced. 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 P I see 3 reason why the fallback could be useful: 1: to be able to work with a version of Sentry that doesn't support FilteredPrivilegeCache 2: to be able to avoid the memory cost of the PrincipalPrivilegeTree 3: to be able to turn this off if it turns out to be buggy I think that 1 is not a valid scenario, as Impala wouldn't even compile with such a Sentry version. I expect this patch to be backported together with the relevant Sentry changes. 2 makes sense as the tree does consume some extra memory for privileges, but I assume that most users who have enough privileges for this to matter will also have problems with the slow SHOW DATABASES / TABLES. (this is not necessarily true, as Impala caches privileges for every user/role, while the speed is only affected by the number of privileges of the current user and its roles). I am not enthusiastic about 3, as adding an option could also introduce issues (there were some Sentry bugs that only occurred if it had to fall back to PrivilegeCache, see SENTRY-2545 and SENTRY-2549), so I am not sure that we would really help here. Actually SENTRY-2549 is still not merged to ASF/master Sentry, so falling back to PrivilegeCache is still buggy. So I think that 2 is the only good reason, but if we want to spend time to reduce the mem cost/privilege, I would prefer other ways, e.g. interning database/table names in privileges (we already intern these string in other CatalogObjects). 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? I added some explanation in PrincipalPrivilegeTree. +1 reason is that URIs are also a bit more problematic than other objects due to their case sensitivity. 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@200 PS7, Line 200: > Oops, I forgot this, I will do this in the next patch. Done -- 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: 11 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 16:37:57 +0000 Gerrit-HasComments: Yes
