Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests ...................................................................... Patch Set 2: (6 comments) I like the new format. Few first pass comments. http://gerrit.cloudera.org:8080/#/c/10135/2/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/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95 PS2, Line 95: ); Shouldn't there also be : .error ("USER....", onServer(TPrivilegeLevel.REFRESH, TPrivilegeLevel.CREATE,TPrivilegeLevel.ALTER,TPrivilegeLevel.DROP)) .error ("USER....", onDatabase("functional", TPrivilegeLevel.REFRESH, TPrivilegeLevel.CREATE,TPrivilegeLevel.ALTER,TPrivilegeLevel.DROP)) .etc.... for onServer, Database, Table, Column, and for the other tests as well to ensure none of the other privileges allow the statement? http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@118 PS2, Line 118: // TODO: IMPALA-6718 : //.ok(onColumn("functional", "alltypes_view", new String[]{"id", "bool_col", : // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col", : // "double_col", "date_string_col", "string_col", "timestamp_col", "year", : // "month"}, TPrivilegeLevel.SELECT)) : //.ok(onColumn("functional", "alltypes_view", new String[]{"id", "bool_col", : // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col", : // "double_col", "date_string_col", "string_col", "timestamp_col", "year", : // "month"}, TPrivilegeLevel.SELECT)) Take out this todo as it's waiting on future function. When the function is delivered, then appropriate tests should be added. Add a comment to the Impala-6718 Jira. http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@138 PS2, Line 138: // TODO: IMPALA-6718 : //.ok(onColumn("functional", "alltypes_view", "id", TPrivilegeLevel.SELECT)) Remove. Add comment to Jira. http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@325 PS2, Line 325: // TODO: IMPALA-6891 : //.ok(onColumn("functional", "alltypes", new String[]{"id", "bool_col", : // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col", : // "double_col", "date_string_col", "string_col", "timestamp_col", "year", : // "month"}, TPrivilegeLevel.SELECT), : // onColumn("functional", "alltypessmall", new String[]{"id", "bool_col", : // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col", : // "double_col", "date_string_col", "string_col", "timestamp_col", "year", : // "month"}, TPrivilegeLevel.SELECT)) Remove. Add a comment to the Jira. http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@388 PS2, Line 388: TPrivilege[]... privileges There doesn't seem to be any tests that use this. If the other tests from comments above are not needed, then is this? http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509 PS2, Line 509: ) Since we're changing this anyway, can we add another method that takes a boolean to indicate it should use the entire string instead of just starts with? Some of the authorization errors are important to distinguish that the error returns just the table, not the table and column. -- 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: 2 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: Tue, 24 Apr 2018 06:00:59 +0000 Gerrit-HasComments: Yes
