ji chen 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: (2 comments) Hi, Fang Yu, Thank you for reviewing my patch, I need to instantiate 'authzCatalog_' by an instance of CatalogServiceTestCatalogWithRanger is because there are some behaviour difference for role creation and drop, between the unit test and e2e test. in the e2e test, method createRole/dropRole in CatalogOpExecutor will be called, these methods will directly create/drop role in ranger backend, in other words,the roles will be synchronized to ranger service, however, in unit tests, method addRole/removeRole in CatalogServiceCatalog will be called, to manupulate roles, the two method will only add/remove role entry in AuthorizationPolicy instance, which is a principal cache, the role will not be synchronized to ranger service. because in the new implementation, ranger plugin is directly used to check roles, so If CatalogServiceTestCatalog is used, ranger plugin will fails to check the role, because roles are only created in AuthorizationPolicy cache. so I have to override the method addRole/removeRole in class CatalogServiceTestCatalogWithRanger, the implementation will both create role in ranger backend and AuthorizationPolicy cache, so that the unit tests can pass. 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). It is a very good idea,will change as per your suggestion. 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 instantia this change can be reverted, I wrongly thought the original line would exceed 90 characters -- 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-Reviewer: ji chen <[email protected]> Gerrit-Comment-Date: Fri, 29 Sep 2023 16:12:03 +0000 Gerrit-HasComments: Yes
