Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20508 )
Change subject: IMPALA-12398: Ranger role not exists when altering db/table/view owner to a role ...................................................................... Patch Set 3: (6 comments) Thanks for the effort Ji! I took a look at this patch and left some minor comments. One thing that's not that clear to me is why we need to instantiate 'authzCatalog_' by an instance of CatalogServiceTestCatalogWithRanger defined in this proposed patch. This patch resolves the reported issue by implementing the method checkRole() to determine whether a given role exists in the Ranger service and checkRole() is already on the code path exercised by the added test cases. Maybe I missed something important. I will take a second pass to better understand this patch. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/main/java/org/apache/impala/analysis/AnalysisUtils.java File fe/src/main/java/org/apache/impala/analysis/AnalysisUtils.java: http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/main/java/org/apache/impala/analysis/AnalysisUtils.java@51 PS3, Line 51: checkRole This method seems to be more related to authorization (v.s. analysis). Would it be better if we define the method checkRole() in the interface AuthorizationManager () (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java) and then implement the method in RangerImpaladAuthorizationManager? In the future if we are adopting another authorization provider other than Ranger, we could implement checkRole() in another class that extends AuthorizationManager.java like https://gerrit.cloudera.org/c/15833/8/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java that was removed in IMPALA-9708. http://gerrit.cloudera.org:8080/#/c/20508/3/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/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java@129 PS3, Line 129: roleExist nit: Maybe it's better to change the name to roleExists. http://gerrit.cloudera.org:8080/#/c/20508/3/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/3/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@2546 PS3, Line 2546: } nit: Add a newline after the right parenthesis. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@2547 PS3, Line 2547: thrown analysis exception nit: throw an AnalysisException. http://gerrit.cloudera.org:8080/#/c/20508/3/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/20508/3/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java@126 PS3, Line 126: testCatalog_ = CatalogServiceTestCatalogWithRanger.createWithAuth(authzFactory_); Could you briefly explain why we need an instance of CatalogServiceTestCatalogWithRanger to instantiate 'authzCatalog_'? Specifically, what would be different in the frontend authorization tests after we instantiate 'authzCatalog_' as proposed by this patch? 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: Not very sure if I missed something. But what was wrong with this instantiation? -- 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: 3 Gerrit-Owner: ji chen <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 28 Sep 2023 01:09:03 +0000 Gerrit-HasComments: Yes
