Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support ...................................................................... Patch Set 5: > Hi Joe, thank you for the effort to remove Sentry support! > > I do not have any major comment but the following very minor one. > > After removing Sentry support, there is no role-based authorization > provider in Impala, meaning that some statement like "SHOW ROLES" > is no longer supported. In this regard, I was wondering whether we > also need to remove the related code. > > For instance, in the case of "SHOW ROLES" described above, we may > remove the method of getRoles() at > 1) > https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/JniFrontend.java#L531-L541, > 2) > https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java#L54, > and > 3) > https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java#L99-L107. > > Also, some production rule like show_roles_stmt at > https://github.com/apache/impala/blob/master/fe/src/main/cup/sql-parser.cup#L944-L951 > and ShowRoleStmt at > https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java > may have to be removed as well if we decide to completely remove > support for role-based authorization. > > But on the other hand, it seems better or more flexible to keep > those statements for role-based authorization in case we would like > to add support for role-based authorization (other than Sentry) > some day and I guess that is the reason why you chose not to remove > these statements for role-based authorization. This is something I didn't consider. I think if we don't have any authorization that supports roles and Ranger isn't planning on supporting roles, then we should consider removing the roles functionality. I think we can turn that into a separate change. I will file a JIRA for tracking removing the show roles functionality. We can have a discussion on that JIRA. I imagine there will be several followup cleanups as we find out what is no longer needed. -- To view, visit http://gerrit.cloudera.org:8080/15833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e Gerrit-Change-Number: 15833 Gerrit-PatchSet: 5 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 19 May 2020 00:04:43 +0000 Gerrit-HasComments: No
