Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5489: Improve Sentry authorization for Kudu tables ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/7307/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java: Line 131: formatArgs)); > Add a test case for all privileges? Done http://gerrit.cloudera.org:8080/#/c/7307/1/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test File testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test: Line 696: create role grant_revoke_test_KUDU > Would it be preferable to create a separate file for Kudu grant revoke test Actually, I need to split it into another file, not to reduce test time (actually w/ a custom cluster test it's more expensive to run more tests in most cases), but because Kudu isn't supported on some OSes and some of these statements will fail. I have another patch which needs to go in first where I'll do the work to split this out. I'll rebase this patch on top of that one later. PS1, Line 712: ---- QUERY : insert into grant_rev_db.test_kudu_tbl values (1, "foo"); : ==== : ---- QUERY : upsert into grant_rev_db.test_kudu_tbl values (1, "bar"); > Maybe you also want to run these statements before granting insert to show Done PS1, Line 716: upsert per the discussion about ALL, maybe UPSERT should actually require ALL for now - until we have an UPSERT privilege explicitly. What do you think? PS1, Line 737: SELECT > hm, why 'SELECT'? you pointed out the reason below. However, I've changed it to be ALL. PS1, Line 740: GRANT SELECT(a) ON TABLE grant_rev_db.test_kudu_tbl TO grant_revoke_test_KUDU : ---- RESULTS : ==== : ---- QUERY : update grant_rev_db.test_kudu_tbl set a = "zzz" > This is kind of weird to me. What if you had a where clause in the update s I think it kinda makes sense but I'll make it require ALL because the upgrade case when we support more fine grained privs will make more sense. Line 804: drop role grant_revoke_test_ROOT; > Don't you need to clean up the role here? Done -- To view, visit http://gerrit.cloudera.org:8080/7307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib12d2b32fa3e142e69bd8b0f24f53f9e5cbf7460 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
