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 <fwij...@cloudera.com>
Gerrit-Reviewer: Adam Holley <g...@holleyism.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 04:31:42 +0000
Gerrit-HasComments: Yes

Reply via email to