Vuk Ercegovac 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 isObjectOwnershipEnabled is false. What will be the end-state of the various systems under the four combinations here? 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, e.g., (..., /*tableName*/null, ...), in particular for nulls and booleans. 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, one will go first and then the other will overwrite)... I see that addUser will overwrite so the perhaps the race is a no-op. however, it differs from the intent here so just wondering if anything unintended crops up here? for example, two operations for the same user will succeed so externally, will it look like the same user was added twice? if there is an assumption about locking from the caller, pls state it. 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? 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 analysis? why let one of those alter commands get to this point when sentry object ownership is not enabled? same question for the method below as well. if the main explanation is that something is optional here-- is that spelled out somewhere? 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? 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 pertain to "nogrant". pls add the same to the just "oo" sentry file as well. I have no idea which of these specify the intent of the file. 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 ImpalaTestSuite? 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? 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 matrix? I see that all tests are marked to run serially, but in any event, I'm worried about leakage to other tests, or within this test. 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? 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) will be cleaned up. The name can be passed down into __execute_owner.._tests. Not sure if you tried that and ran into some issues. 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? 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? http://gerrit.cloudera.org:8080/#/c/11314/23/tests/authorization/test_owner_privileges.py@255 PS23, Line 255: nit: ws 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? -- 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: Thu, 06 Sep 2018 21:41:52 +0000 Gerrit-HasComments: Yes
