Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java: http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47 PS5, Line 47: import static org.junit.Assert.fail; > I tried running the test and it feels very slow. Do you know what's making Done. Avoiding calling catalog_.reset() and making Sentry RPCs makes the whole tests faster in a significant way. http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@75 PS5, Line 75: > Why do we need this? To ensure we have existing roles created externally will not interfere with these tests. I'll add some comment in the code. http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@122 PS5, Line 122: // Select a specific column on a table. > What does this mean? My understanding is that columns can only have SELECT, Done. It's allowed in Sentry but not in Impala. http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@157 PS5, Line 157: .error("User '%s' does not have privileges to execute 'SELECT' on: " + > add comment that column-level privs are currently not supported Done http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@183 PS5, Line 183: TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT))) > I feel like there's still a lot of redundancy in these tests, could we perh Done http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@324 PS5, Line 324: "functional.alltypes", onColumn("functional", "alltypes", "id", allExcept( > also very similar to the other test blocks, can we condense them further? Done http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349 PS5, Line 349: "functional.alltypes", onTable("functional", "alltypes", allExcept( > condense further? Shouldn't this be a separate test case since this is to test a table with an alias? http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@451 PS5, Line 451: "month"}, allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT))); > I feel like these tests could be structured a little more. Not sure if you Done. I restructured the tests. Let me know what you think. -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Comment-Date: Thu, 26 Apr 2018 02:47:05 +0000 Gerrit-HasComments: Yes
