Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9589 )
Change subject: IMPALA-6643: Add REFRESH fine-grained privilege ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@116 PS2, Line 116: analyzer.registerPrivReq(new PrivilegeRequest(Privilege.ALL)); Shouldn't the refresh privilege be applied to this case as well? i.e. onServer() Might require an e2e test instead of AuthorizationTest. http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/main/java/org/apache/impala/authorization/Privilege.java File fe/src/main/java/org/apache/impala/authorization/Privilege.java: http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/main/java/org/apache/impala/authorization/Privilege.java@50 PS2, Line 50: SentryAction Is this the best name? Since Sentry does not care what the string is, and it's more specific to SQL. Refresh allows the invalidate metadata action. This list is really privileges. Maybe SQLPrivilege? Also, should this be it's own file? I know there's cases for either way in the code. http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java: http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@236 PS2, Line 236: Update comment at top of file to reflect new setup. http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@249 PS2, Line 249: roleName = "refresh_functional_text_lzo"; : sentryService.createRole(USER, roleName, true); : sentryService.grantRoleToGroup(USER, roleName, USER.getName()); Does this chunk need to be duplicated? http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@261 PS2, Line 261: roleName = "refresh_functional_text_lzo"; : sentryService.createRole(USER, roleName, true); : sentryService.grantRoleToGroup(USER, roleName, USER.getName()); duplicate? http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@273 PS2, Line 273: roleName = "refresh_functional_text_lzo"; : sentryService.createRole(USER, roleName, true); : sentryService.grantRoleToGroup(USER, roleName, USER.getName()); duplicate? http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@3579 PS2, Line 3579: // REFRESH privilege object. I think we need refresh on SERVER also. http://gerrit.cloudera.org:8080/#/c/9589/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@3580 PS2, Line 3580: ParsesOk(String.format("%s REFRESH ON DATABASE foo %s myRole", formatStr)); There's no current method to "REFRESH" or "INVALIDATE METADATA" at the database level, only server and table. -- To view, visit http://gerrit.cloudera.org:8080/9589 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c3c5a51fe493d39fd719c7a388d4d5760049ce4 Gerrit-Change-Number: 9589 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Comment-Date: Tue, 13 Mar 2018 04:31:42 +0000 Gerrit-HasComments: Yes
