Fang-Yu Rao 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) 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()) { > I think refactoring RangerAuthorizationChecker may need a refactor on Autho Thanks Quanlong for the detailed explanation! My original thought is to bind UserGroupInformation with RangerAuthorizationChecker only. According to my current understanding, both Sentry and Ranger may require the group information of the requesting user when performing the authorization. The difference is that Sentry would consult the group mapper with the group information itself, whereas Ranger does not. More specifically, it is Impala's responsibility to retrieve the corresponding group information from an instance of UserGroupInformation and then pass the group information to Ranger. But I agree with you. We may need more detailed discussion before implementing my proposed approach and we may do it in a follow-up patch. 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, G Thanks Quanlong! I have changed 'USER' to 'user_'. But do we have to change 'OWNER_USER' to 'owner_user_'? 'OWNER_USER' is currently a final variable. 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" Done 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 Done 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 Thanks for catching this! I have change the line above to getName(). -- 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 18:39:10 +0000 Gerrit-HasComments: Yes
