Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16837 )
Change subject: IMPALA-10211 (Part 1): Add support for role-related statements ...................................................................... Patch Set 4: (12 comments) Nice work! Just gone through the implementation, haven't looked into the tests in details yet. The implementation makes sense to me. http://gerrit.cloudera.org:8080/#/c/16837/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16837/4//COMMIT_MSG@12 PS4, Line 12: GROUP <group_name> Just curious, will we support grant role to user? http://gerrit.cloudera.org:8080/#/c/16837/4//COMMIT_MSG@56 PS4, Line 56: there is no API provided by Ranger that allows Impala to : directly retrieve the list of all privileges granted to a specified : role. Just curious, is this in the roadmap of Ranger, and Is there a Ranger JIRA about this? I think SHOW GRANT ROLE <role_name> without specifying the resource is useful, especially when there are lots of dbs/tables, it's painful to iterate on all possible resources to know what a role can do. As the first item mentioned, to drop a role we have to know all its privileges and drop them first. http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@85 PS4, Line 85: null, null, null, null, null nit: These nulls are not good for readability. I think this is better: RangerRole role = new RangerRole(); role.setName(params.getRole_name()); http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@89 PS4, Line 89: null nit: I think our code style is adding a short comment for null parameter values, i.e. plugin_.get().createRole(role, /*resultProcessor*/ null); http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@91 PS4, Line 91: LOG.error("Error creating a role in Ranger. " + : requestingUser.getShortName(), e); nit: LOG.error("Error creating role {} in Ranger.", requestingUser.getShortName(), e); http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@95 PS4, Line 95: } Should we call refreshAuthorization(response) at the end? http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@105 PS4, Line 105: row nit: typo? row -> role? http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@115 PS4, Line 115: } Should we call refreshAuthorization(response) at the end here? http://gerrit.cloudera.org:8080/#/c/16837/4/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/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@22 PS4, Line 22: import com.google.common.base.Preconditions; nit: keep the import list sorted http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@136 PS4, Line 136: roleNames = toRoleNames(roles); nit: we can simplify the toRoleNames method as roleNames = roles.stream().map(RangerRole::getName).collect(Collectors.toSet()); http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@141 PS4, Line 141: result.setRole_names(Lists.newArrayListWithExpectedSize(roleNames.size())); : result.getRole_names().addAll(roleNames); Can we use result.setRole_names(Lists.newArrayList(roleNames)) directly? http://gerrit.cloudera.org:8080/#/c/16837/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@147 PS4, Line 147: LOG.error("Error executing SHOW CURRENT ROLES."); nit: It'd be useful to log the exception as well, i.e. LOG.error("Error executing SHOW CURRENT ROLES.", e); -- To view, visit http://gerrit.cloudera.org:8080/16837 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b204e62a1d8ae1932d955b4efc28be22202860 Gerrit-Change-Number: 16837 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Fri, 11 Dec 2020 08:35:28 +0000 Gerrit-HasComments: Yes
