Fang-Yu Rao 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: Code-Review+1 (3 comments) Very sorry for my late response and thanks a lot for addressing most of my comments on the patch set 3! It's great we do not have duplicate code in CatalogServiceTestCatalogWithRanger.java. :-) I only have one very minor suggestion (adding a test case for the ALTER DATABASE SET OWNER ROLE statement) and do not have any other comment. http://gerrit.cloudera.org:8080/#/c/20508/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/20508/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@2548 PS7, Line 2548: ALTER VIEW SET OWNER ROLE Could we also add a test case for the ALTER DATABASE SET OWNER ROLE statement? I am asking since we also check the existence of a role at https://gerrit.cloudera.org/c/20508/7/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java#63. http://gerrit.cloudera.org:8080/#/c/20508/3/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/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@96 PS3, Line 96: // still exists in plugin cache : rangerImpalaPlugin_.refreshPoliciesAndTags(); : authRole = super.removeRole(roleName); : } catch (Exception ex) { : ex.printStackTrace(); : } : return authRole; : } : } : : : > Hi, Fang-Yu Rao, In our test cases, the set grantGroups is always empty, th Thanks for the explanation Ji! http://gerrit.cloudera.org:8080/#/c/20508/3/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/3/fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java@a75 PS3, Line 75: > this change can be reverted, I wrongly thought the original line would exce Thanks for the explanation! -- 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 <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: ji chen <[email protected]> Gerrit-Comment-Date: Fri, 17 Nov 2023 03:35:49 +0000 Gerrit-HasComments: Yes
