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 2: (6 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 : I don't know if we want to go through every permutation in the error. It can be somewhat too excessive. The ok() ensures we have the minimum requirement. For example: .ok(onTable("functional", "alltypes", TPrivilegeLevel.SELECT)) means we must have SELECT privilege on functional.alltypes and without it, we will get an authorization error. 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 i I saw some existing code that adds TODO with a ticket number to give more context especially if this is a bug found due to this CR. I'm okay with removing it, though. What do others think? 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. Same comment as above. 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. Same comment as above. 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 It's not used for the first part of this patch. In the next part like if we want to do ALTER DATABASE RENAME, we need a CREATE privilege on a database and an ALTER privilege on a table, this method can be handy. 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 bo Passing a full error string in expectedErrorString is essentially comparing the entire string. I can overload it, but then we need to overload bunch authzError methods. -- 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:22:33 +0000 Gerrit-HasComments: Yes
