Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20916 )
Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE ...................................................................... Patch Set 1: Code-Review+1 (4 comments) LGTM. Just have some minor comments. http://gerrit.cloudera.org:8080/#/c/20916/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20916/1//COMMIT_MSG@33 PS1, Line 33: the principal type could be : a group or a role. Will this be supported in future tasks? I.e. a user belongs to the owner group can also grant/revoke privileges. http://gerrit.cloudera.org:8080/#/c/20916/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java: http://gerrit.cloudera.org:8080/#/c/20916/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@236 PS1, Line 236: Preconditions.checkState(!Strings.isNullOrEmpty(dbName_)); : try { : analyzeTargetDatabase(analyzer); : } catch (AnalysisException e) { : throw new AnalysisException(String.format("Error setting/showing privileges " + : "for database '%s'. Verify that the database exists and that you have " + : "permissions to issue a GRANT/REVOKE/SHOW GRANT statement.", dbName_)); : } nit: We can move codes of the precondition check and the try-catch clause into analyzeTargetDatabase(). Just like what we did for analyzeTargetTable(). http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py@1191 PS1, Line 1191: for privilege in privileges: > optional: I think that it would be easier to digest this function if some s +1. This test has ~300 lines. It'd be better to refactor it. http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py@1198 PS1, Line 1198: admin_client.execute("refresh authorization", user=ADMIN) nit: do we really need this refresh? I thought we only need it when we add/modify/delete policies externally (e.g. in the Ranger WebUI). -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[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: Fri, 26 Jan 2024 01:10:51 +0000 Gerrit-HasComments: Yes
