Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11314 )
Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER ...................................................................... Patch Set 18: (17 comments) http://gerrit.cloudera.org:8080/#/c/11314/18/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/11314/18/common/thrift/JniCatalog.thrift@77 PS18, Line 77: // The server name for security privileges when authorization enabled > nit: most of the other comment sentences are finished with "." - it could b Done http://gerrit.cloudera.org:8080/#/c/11314/18/common/thrift/JniCatalog.thrift@335 PS18, Line 335: // The name of the server for security. > Other server_name members have different comments - is there a reason for t Just missed an update. Done. http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java: http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java@57 PS18, Line 57: > nit: extra line Done http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java@67 PS18, Line 67: if (serverName_ != null) setOwnerParams.setServer_name(serverName_); > The "if" part is not needed here, as null indicates "not set" for non-primi Done http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java File fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java: http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java@20 PS18, Line 20: import com.google.common.base.Preconditions; > Is this still needed? I do not see any new Precondition. Done http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1006 PS18, Line 1006: if (newDb != null) { > "newDb" cannot be null at this point, see precondition at line 996. Done http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1011 PS18, Line 1011: addPrivilegeToCatalog(newDb.getMetaStoreDb().getOwnerName(), > Is it ok that metastoreDdlLock_ is not held at this point? I see that some Meant to put these outside of locks. Updated dropTableOrView and createTable. http://gerrit.cloudera.org:8080/#/c/11314/18/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1877 PS18, Line 1877: privilege > This could be made a bit more compact by using Thrift setters in the fluent Done http://gerrit.cloudera.org:8080/#/c/11314/18/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11314/18/tests/authorization/test_owner_privileges.py@99 PS18, Line 99: self.execute_query("drop role owner_priv_admin") > If line 105 can throw an exception, can't this line drop one too? 105 could throw one because if we try to cleanup after partial or failed tests we want to ignore it. But with this one we shouldn't ignore it because that indicates something else was wrong. http://gerrit.cloudera.org:8080/#/c/11314/18/tests/authorization/test_owner_privileges.py@106 PS18, Line 106: e > flake8: F841 local variable 'e' is assigned to but never used Done http://gerrit.cloudera.org:8080/#/c/11314/18/tests/authorization/test_owner_privileges.py@210 PS18, Line 210: result = self.__root_query("show grant user root") : assert len(result.data) == 0 : : # Ensure root does not have user privileges. : result = self.__root_query("show grant user root") : assert len(result.data) == 0 > Looks like duplication. It is. Done. http://gerrit.cloudera.org:8080/#/c/11314/18/tests/authorization/test_owner_privileges.py@252 PS18, Line 252: __check_no_privileges > Can you explain the difference between this and __check_privileges? My impr I had to refactor this and remove this method. Current "show grant user" only shows privileges for the user, so when none exists, it will throw an exception. There is an open Jira to fix the behavior to not throw and exception, and to also include privileges through roles for the user. When that Jira is fixed, these tests will need to be updated, but needs to be done after this Jira. http://gerrit.cloudera.org:8080/#/c/11314/18/tests/authorization/test_owner_privileges.py@275 PS18, Line 275: null or the timeout_sec is reached. If exclusion is true, the ensure they don't > typo: then Done http://gerrit.cloudera.org:8080/#/c/11314/18/tests/authorization/test_owner_privileges.py@345 PS18, Line 345: # Ensure grant doesn't work. : try: > This doesn't fail if the try block doesn't throw any exception. "excepting Refactored to catch tests. This method was calling __root_query which expects success and did not have the full exception message in e. http://gerrit.cloudera.org:8080/#/c/11314/18/tests/authorization/test_owner_privileges.py@365 PS18, Line 365: with ' > I see a bit of inconsistency here: the first two exception messages do not I think the current reason for the difference in messages is 'GRANT OPTION' is an optional flag for a statement while the other executes are for the statement themselves. http://gerrit.cloudera.org:8080/#/c/11314/18/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/11314/18/tests/common/custom_cluster_test_suite.py@121 PS18, Line 121: if SENTRY_CONFIG in method.func_dict: : self._start_sentry_service(method.func_dict[SENTRY_CONFIG]) > Can this cause side effects in other tests? I mean that custom cluster test The other custom cluster tests use file based authentication. As file based gets deprecated, the tests should be updated to start sentry with their expected config. Maybe a default config can be added with those changes. http://gerrit.cloudera.org:8080/#/c/11314/18/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/11314/18/tests/common/impala_test_suite.py@537 PS18, Line 537: > nit: +2 indentation needed Done -- To view, visit http://gerrit.cloudera.org:8080/11314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0 Gerrit-Change-Number: 11314 Gerrit-PatchSet: 18 Gerrit-Owner: Adam Holley <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 03 Sep 2018 21:12:46 +0000 Gerrit-HasComments: Yes
