Quanlong Huang 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 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/20439/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/20439/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@141 PS2, Line 141: } catch (NullPointerException ex) { It's not a best practise to catch NullPointerException. We can check whether 'roles' is null here, and set roleNames to an empty set if 'roles' is null. http://gerrit.cloudera.org:8080/#/c/20439/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@142 PS2, Line 142: EMPTY_ROLES nit: use Collections.emptySet() directly. http://gerrit.cloudera.org:8080/#/c/20439/2/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/2/fe/src/test/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManagerTest.java@46 PS2, Line 46: } nit: add a blank line after this http://gerrit.cloudera.org:8080/#/c/20439/2/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20439/2/tests/authorization/test_ranger.py@2318 PS2, Line 2318: for statement in show_roles_statements: : result = self.execute_query_expect_success(admin_client, statement, : user=ADMIN) : print("{0}=>{1}\n".format(statement, result)) We use 2 spaces indent. Please check the result instead of just printing it. for statement in show_roles_statements: result = self.execute_query_expect_success( admin_client, statement, user=ADMIN) assert len(result.data) == 0 BTW, I'm not sure if other tests will create roles. If so, we need to make sure they drop the roles at the end. So in this test we can see no roles there. -- 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: 2 Gerrit-Owner: ji chen <jichen0...@163.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Thu, 31 Aug 2023 02:25:17 +0000 Gerrit-HasComments: Yes
