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 40: (6 comments) http://gerrit.cloudera.org:8080/#/c/11314/37/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/37/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2918 PS37, Line 2918: response.result.addToUpdated_catalog_objects(cPrivilege.toTCatalogObject()); > I'm somewhat concerned that you modify owner (which is a shared object) wit Done http://gerrit.cloudera.org:8080/#/c/11314/38/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/38/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2903 PS38, Line 2903: > adding Done http://gerrit.cloudera.org:8080/#/c/11314/38/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2904 PS38, Line 2904: filter.setPrincipal_id(owner.getId()); > it also add a user if specifying a user and that user does not exist. Done http://gerrit.cloudera.org:8080/#/c/11314/40/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/40/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1341 PS40, Line 1341: updateOwnerPrivileges > if this is *not* done for drop, what is the harm in waiting for the next up If we don't drop and a different user creates the object with the same name, the old user will have privileges. http://gerrit.cloudera.org:8080/#/c/11314/40/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1493 PS40, Line 1493: if (table.getMetaStoreTable() != null) { > same question here (why do this for drop). Same reason. Don't want to leave ghost privileges around in case a new object with the same name is created. http://gerrit.cloudera.org:8080/#/c/11314/40/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/11314/40/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2218 PS40, Line 2218: authzCatalog_.addRole("foo_owner"); > confused by this one... after this line, "foo_owner" role exists so the sub This role was added so that the tests will pass. The test for when roles do not exist result in AnalysisException and didn't add. Next patch will include. -- 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: 40 Gerrit-Owner: Adam Holley <ahol...@cloudera.com> Gerrit-Reviewer: Adam Holley <ahol...@cloudera.com> Gerrit-Reviewer: Anonymous Coward #424 Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Thu, 13 Sep 2018 21:13:17 +0000 Gerrit-HasComments: Yes
