Alex Behm has posted comments on this change. Change subject: IMPALA-4000: Restricted Sentry authorization for Kudu Tables ......................................................................
Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/5047/4//COMMIT_MSG Commit Message: Line 12: - Column level permissions are not supported. ... on Kudu tables (just to be very clear) Line 13: - Permissions on certain operations (e.g. only SELECT) are not Move this point to the top and rephrase to: - Only the ALL privilege level can be granted to Kudu tables. Finer-grained levels such as only SELECT or only INSERT are not supported. http://gerrit.cloudera.org:8080/#/c/5047/4/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: Line 247: // See https://issues.cloudera.org/browse/IMPALA-4000 for details. no need for the whole link, just "See IMPALA-4000 for details." http://gerrit.cloudera.org:8080/#/c/5047/4/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java: Line 287: // At this time Kudu tables only support the table level ALL privilege. slightly rephrase: We only support the ALL privilege on Kudu tables since many of the finer-grained levels (DELETE/UPDATE) are not available. Line 291: } else if (scope_ == TPrivilegeScope.COLUMN) { logic is a little simpler to me if you make this an independent "if" (and not an "else if") http://gerrit.cloudera.org:8080/#/c/5047/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java: Line 164: AnalysisError(String.format( Write a loop somewhere that goes over all privilege levels to make sure they all fail. Line 204: AnalysisError(String.format("%s SELECT (id, bool_col) ON TABLE " + use ALL here to get the other err msg http://gerrit.cloudera.org:8080/#/c/5047/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java: Line 82: public class AuthorizationTest { I think we also need to check that a Sentry policy file with a non-ALL on a Kudu table is not valid. I'll need to look into this a little more, but you can do so as well concurrently. Line 2168: // IMPALA-4000: Only users with ALL privileges on SERVER may create external Kudu remove comment (this is a test for admin users) -- To view, visit http://gerrit.cloudera.org:8080/5047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
