ji chen 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 7: (15 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: > nit: we use a limit of 72 characters per line in the commit message. Done http://gerrit.cloudera.org:8080/#/c/20508/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20508/6//COMMIT_MSG@6 PS6, Line 6: : IMPALA-12398 > Wrong JIRA id here. Thanks, will fix in the current revision 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 Done http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@118 PS5, Line 118: > nit: check role for what? Maybe 'roleExists' is a better name Done 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: > Why we need this parameter of 'accessUser'? the parameter accessUser is reserved for now, in cases of checking role considering the user who access it. 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 false; : return roleSet.stream().anyMatch(r -> r.getName().equals(roleName)); : } : } : : : : : : : : : > nit: can be simplified as Done http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java@143 PS5, Line 143: : : : : : : : : : > Do we use these two methods somewhere? it is not used,will remove it in the next version 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: : rangerImpalaPlugin_.createRole(role, null); : rangerImpalaPlugin_.refreshP > nit: return it directly done http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@92 PS5, Line 92: try { > nit: it's already null (initialized at L70). Done http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@94 PS5, Line 94: rImpal > nit: don't need the cast ok http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@106 PS5, Line 106: > nit: unused variable Done http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@109 PS5, Line 109: > nit: it's already initialized at L99 Done 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? this is a dummy AuthorizationFactory implementation, so for legacy compatibility, still use the old code path. 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 Done http://gerrit.cloudera.org:8080/#/c/20508/5/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java@74 PS5, Line 74: () -> new CatalogServiceTestCatalog(false, 16, new MetaStoreClientPool(0, 0))); > nit: add a blank line Done -- 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: 7 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: Wed, 08 Nov 2023 07:56:44 +0000 Gerrit-HasComments: Yes
