Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support ...................................................................... Patch Set 5: Code-Review+1 > 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. Thanks Joe for tracking this! Will try to provide some ideas about the followup cleanups in that JIRA. I do not have any other comment. -- 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 15:03:11 +0000 Gerrit-HasComments: No
