Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14798 )
Change subject: IMPALA-9149: part 1: Re-enabe Ranger-related FE tests ...................................................................... Patch Set 7: (5 comments) Thanks for digging into these, Fang-Yu! > Hi Kurt and Quanlong, I left some additional thoughts on how we should pass > an instance of group mapper to RangerAuthorizationChecker.java. Will try to > implement the proposed approach in the next iteration since I think the > current approach is too hacky and less flexible. Thanks! I think we can make the refactor in later patches since it doesn't look like simple. And we have some other on-going works (IMPALA-9222, IMPALA-9231) about the privilege checks. It'd be better to not disable Ranger tests for too long, in case we introduce bugs in those works. The patch looks good to me overall. Thanks! http://gerrit.cloudera.org:8080/#/c/14798/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/14798/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@314 PS7, Line 314: if (RuntimeEnv.INSTANCE.isTestEnv()) { > After some thoughts, I feel like my approach here is a bit too hacky. A bet I think refactoring RangerAuthorizationChecker may need a refactor on AuthorizationChecker too. It may not be a simple refactor: * Currently, the Frontend class holds a field for AuthorizationChecker ( https://github.com/apache/impala/blob/313d758/fe/src/main/java/org/apache/impala/service/Frontend.java#L256). It's shared by different requests by different users. * To bind User/UserGroupInformation with AuthorizationChecker, we may need to create a AuthorizationChecker instance for each requests. It's cheap for Ranger, but expensive for Sentry. The constructor of SentryAuthorizationChecker finally invokes a java refrection which is expensive (https://github.com/apache/impala/blob/313d758/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthProvider.java#L61). http://gerrit.cloudera.org:8080/#/c/14798/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java: http://gerrit.cloudera.org:8080/#/c/14798/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java@87 PS7, Line 87: USER Rename to user_ since it's now a non-final variable. Same for OWNER_USER, GROUPS, OWNER_GROUPS, AS_OWNER below. http://gerrit.cloudera.org:8080/#/c/14798/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java@266 PS7, Line 266: // Depending on whether or not the principal is the owner of the resource, we return nit: we usually put the comments above the "@Override" http://gerrit.cloudera.org:8080/#/c/14798/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java@270 PS7, Line 270: AS_OWNER == true don't need "== true" since it's boolean http://gerrit.cloudera.org:8080/#/c/14798/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java@282 PS7, Line 282: (AS_OWNER ? OWNER_USER.getName() : USER.getName()) use getName() instead -- To view, visit http://gerrit.cloudera.org:8080/14798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I228533aae34b9ac03bdbbcd51a380770ff17c7f2 Gerrit-Change-Number: 14798 Gerrit-PatchSet: 7 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 13 Dec 2019 09:29:38 +0000 Gerrit-HasComments: Yes
