Csaba Ringhofer 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: (16 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 be also added here and other new comments for the sake of consistency. 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 this? 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 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-primitive types. I would prefer to remove it (here and in other files) to make the code more compact and consistent. 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. 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. 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 functions (dropTableOrView, createTable) do this operation inside the synchronized block, while others do not. 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 way. 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? 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. 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 impression is that both will fail if there is any row (=privilege) where one of the parameters do not match. The only difference seems to be is that __check_privileges also checks 'grant_option". 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 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 exceptions" can be done in pytest like this: with pytest.raises(ExceptionsClass) as e: (logic that should throws exception) (logic that verifies exeception 'e') For more details, see https://docs.pytest.org/en/latest/assert.html 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 have ' around that statement. + this one has a "with", while others do not. 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 tests without SENTRY_CONFIG will use the Sentry service started by the last test (if I do not misunderstand something). Can the non-default Sentry params affect test behavior? 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 -- 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 16:41:13 +0000 Gerrit-HasComments: Yes
