Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests ...................................................................... Patch Set 3: Code-Review+1 (6 comments) updated. carry +1 http://gerrit.cloudera.org:8080/#/c/10442/3/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/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@918 PS3, Line 918: @Test > perhaps add a comment explaining what's being tested... from a first glance Done http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@922 PS3, Line 922: > consistent spacing (see comment below) Done http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@938 PS3, Line 938: > and several more places below Done http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@946 PS3, Line 946: allExcept( : TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT) > factor out (repeated 3 times in this block, so it adds too much noise). This is the pattern we've been using in these tests to be explicit for each test on the privileges required. It adds text, but reduces the need to reference what the variable or constant means in a different part of the code. http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@952 PS3, Line 952: ng[]{"id"}, > explain what is being tested here (missing why "id" is special) Done http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1181 PS3, Line 1181: > use consistent spacing for this... from a brief look, seems like no space i Done -- To view, visit http://gerrit.cloudera.org:8080/10442 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4 Gerrit-Change-Number: 10442 Gerrit-PatchSet: 3 Gerrit-Owner: Adam Holley <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 12 Jun 2018 21:48:18 +0000 Gerrit-HasComments: Yes
