Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20439 )
Change subject: IMPALA-12397: NullPointerException in SHOW ROLES when there are no roles ...................................................................... Patch Set 3: (4 comments) Thanks for the patch and the new test cases Ji! I only have some very minor comments. http://gerrit.cloudera.org:8080/#/c/20439/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java File fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java: http://gerrit.cloudera.org:8080/#/c/20439/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java@48 PS3, Line 48: showRolesParams.setRequesting_user("admin"); nit: It seems that the value of 'requesting_user' does not have to be "admin" when 'isShowCurrentRole' is true. In RangerImpaladAuthorizationManager#getRoles(), the operation would be considered as non-administrative for the SHOW CURRENT ROLES command. boolean adminOp = !(groups.contains(params.getGrant_group()) || params.is_show_current_roles); if (adminOp) { RangerUtil.validateRangerAdmin(plugin_.get(), params.getRequesting_user()); } http://gerrit.cloudera.org:8080/#/c/20439/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java@50 PS3, Line 50: } nit: Add a newline after this right curly bracket. http://gerrit.cloudera.org:8080/#/c/20439/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java@64 PS3, Line 64: used nit: thrown http://gerrit.cloudera.org:8080/#/c/20439/3/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20439/3/tests/authorization/test_ranger.py@2304 PS3, Line 2304: reset_ranger=True Thanks! I think this addresses Quanlong's comment at https://gerrit.cloudera.org/c/20439/2/tests/authorization/test_ranger.py#2321. When 'reset_ranger' is True, we reset all the policies in the Ranger service and thus even if there were roles before this test, they will be deleted when this test runs. It may be a good idea to add a code comment stating that since the Ranger policies are reset before this test, we do not have to worry about there could be existing roles when the test is running. -- To view, visit http://gerrit.cloudera.org:8080/20439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id80fc2c9152a09194718da1b4266c5f804f0971f Gerrit-Change-Number: 20439 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: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 06 Sep 2023 23:05:10 +0000 Gerrit-HasComments: Yes
