Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20508 )
Change subject: IMPALA-12398: Fix Ranger role not exists when altering db/table/view owner to a role ...................................................................... Patch Set 5: (14 comments) http://gerrit.cloudera.org:8080/#/c/20508/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20508/5//COMMIT_MSG@21 PS5, Line 21: This patch will directly use ranger impala plugin to check the existence of the role, nit: we use a limit of 72 characters per line in the commit message. You can add this in your .vimrc to auto format each line au FileType gitcommit setlocal tw=72 http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@114 PS5, Line 114: void postAnalyze(AuthorizationContext authzCtx); nit: add a blank line here http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@118 PS5, Line 118: checkRole nit: check role for what? Maybe 'roleExists' is a better name http://gerrit.cloudera.org:8080/#/c/20508/5/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/20508/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@759 PS5, Line 759: accessUser Why we need this parameter of 'accessUser'? http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java: http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java@128 PS5, Line 128: if (roleSet != null) { : return roleSet.stream() : .filter(new Predicate<RangerRole>() { : @Override : public boolean test(RangerRole rangerRole) { : return rangerRole.getName().equals(roleName); : } : }) : .count() : > 0; : } else { : return false; : } nit: can be simplified as if (roleSet == null) return false; return roleSet.stream().anyMatch(r -> r.getName().equals(roleName)); then don't need to import Predicate at line 31. http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java@143 PS5, Line 143: public static boolean roleExists(AuthorizationFactory authzFactory, String roleName) { : RangerAuthorizationChecker rauthzChecker = : (RangerAuthorizationChecker) authzFactory.newAuthorizationChecker(); : RangerImpalaPlugin plugin = rauthzChecker.getRangerImpalaPlugin(); : return roleExists(plugin, roleName); : } : : public static boolean isRangerEnabled(AuthorizationFactory authzFactory) { : return (authzFactory instanceof RangerAuthorizationFactory); : } Do we use these two methods somewhere? http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java File fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java: http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@79 PS5, Line 79: RangerRole.RoleMember roleMember = : new RangerRole.RoleMember(s, s.equals(RANGER_ADMIN_USER)); : return roleMember; nit: return it directly return new RangerRole.RoleMember(s, s.equals(RANGER_ADMIN_USER)); or more simpler: List<RangerRole.RoleMember> roleMemberList = grantGroups.stream() .map(s -> new RangerRole.RoleMember(s, s.equals(RANGER_ADMIN_USER))) .collect(Collectors.toList()); http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@92 PS5, Line 92: authRole = null; nit: it's already null (initialized at L70). http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@94 PS5, Line 94: (Role) nit: don't need the cast http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@106 PS5, Line 106: Role role nit: unused variable http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@109 PS5, Line 109: authRole = null; nit: it's already initialized at L99 http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@445 PS5, Line 445: return catalog_.getAuthPolicy().getRole(roleName) != null; Shouldn't we get it from the ranger plugin? http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java File fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java: http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java@62 PS5, Line 62: } nit: add a blank line http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java@74 PS5, Line 74: } nit: add a blank line -- To view, visit http://gerrit.cloudera.org:8080/20508 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b029bdb90111dbd0eab5189360cc81090225cda Gerrit-Change-Number: 20508 Gerrit-PatchSet: 5 Gerrit-Owner: ji chen <jichen0...@163.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: ji chen <jichen0...@163.com> Gerrit-Comment-Date: Fri, 03 Nov 2023 02:54:36 +0000 Gerrit-HasComments: Yes
