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 23: (16 comments) http://gerrit.cloudera.org:8080/#/c/11314/23/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/23/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java@54 PS23, Line 54: authorization is enabled > I got confused by what's supposed to happen if its enabled, but isObjectOwn Done http://gerrit.cloudera.org:8080/#/c/11314/23/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/23/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1002 PS23, Line 1002: updateDatabasePrivileges(newDb.getName(), null, params.server_name, null, null, > with so many args, it would be easier to read if the params were commented, Done http://gerrit.cloudera.org:8080/#/c/11314/23/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1840 PS23, Line 1840: if (user == null) { > looks like there's a race here (two operations see that owner doesn't exist Done http://gerrit.cloudera.org:8080/#/c/11314/23/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2878 PS23, Line 2878: .removePrivilege(privilege); > is there a race here as well? Duplicate removes should basically noop the second one. http://gerrit.cloudera.org:8080/#/c/11314/23/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3196 PS23, Line 3196: isObjectOwnershipEnabled > after seeing this method, I'm wondering if this can be pulled up to analysi Moved to updateDatabasePrivileges http://gerrit.cloudera.org:8080/#/c/11314/23/fe/src/test/resources/sentry-site_no_oo.xml.template File fe/src/test/resources/sentry-site_no_oo.xml.template: http://gerrit.cloudera.org:8080/#/c/11314/23/fe/src/test/resources/sentry-site_no_oo.xml.template@76 PS23, Line 76: enable owner > in this case, disable? Done http://gerrit.cloudera.org:8080/#/c/11314/23/fe/src/test/resources/sentry-site_oo_nogrant.xml.template File fe/src/test/resources/sentry-site_oo_nogrant.xml.template: http://gerrit.cloudera.org:8080/#/c/11314/23/fe/src/test/resources/sentry-site_oo_nogrant.xml.template@22 PS23, Line 22: <name>sentry.service.security.mode</name> > add comments to highlight which of these settings pertain to "oo" and which Done http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@70 PS23, Line 70: ImpalaTestSuite > is this needed since CustomClusterTestSuite inherits already from ImpalaTes Done http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@81 PS23, Line 81: Using user 'root > I missed this.. where is 'root' explicitly used? Adding a comment at the top to address. http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@87 PS23, Line 87: self.execute_query("create role owner_priv_test_ROOT") > can things be simplified if you create a unique id per test in the test mat Adding a comment at the top to address. Basically need "root" because every system the tests run on should have a user root with group root. http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@109 PS23, Line 109: self.execute_query("create role owner_priv_admin") : > why are these added when cleanup is supposed to remove things? We create a role and grant it privileges so we can clean up otherwise we might not have access. We don't want to rely on roles being in a state at the end of tests. http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@156 PS23, Line 156: def test_owner_privileges_with_grant(self, vector): > if you use the unique_database fixture here, all objects (db, table, view) Can't use that since I want to create the database after the cluster starts up. The unique_database creates the database before the test so owner privileges aren't created. I tried adding 'if not exists' to the create and didn't work. http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@172 PS23, Line 172: self.root_impalad_client = self.create_impala_client() : s > how do these two differ? Adding a comment at the top to address. http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@248 PS23, Line 248: ser="root" > maybe the comment about 'root' up on L81 is relevant here? Adding a comment at the top to address. http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@255 PS23, Line 255: > nit: ws Done http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@398 PS23, Line 398: one of the strings in 'expected_str' > multiple strings in a string? 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: 23 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-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 07 Sep 2018 04:10:28 +0000 Gerrit-HasComments: Yes
