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 33: (8 comments) http://gerrit.cloudera.org:8080/#/c/11314/33/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/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1337 PS33, Line 1337: updateOwnerPrivileges(db.getName(), /* tableName */ null, params.server_name, > It does so via that metastoreDdlLock_ afaict. However, updating privs here My main concern was the locks that were part of the privilege update and having a deadlock because someone gets these in a different order. Since Sentry has it's own locks, I wanted to keep them separate from the HMS locks. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1837 PS33, Line 1837: updateOwnerPrivileges(newTable.getDbName(), newTable.getTableName(), > there are possibly multiple impalads, each with multiple, uncoordinated req But even with multiple impalad that have their own catalog. If this call doesn't return, because of the sleep, the other impalad would not have the object in it's catalog to be able to drop. The catalog update would happen after the create call is done afaik. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2896 PS33, Line 2896: Reference<Boolean> existingUser = new Reference<>(); > this was my suggestion. Done http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3145 PS33, Line 3145: private void grantRevokeRolePrivilege(User requestingUser, > fwict, that lock is acquired when you drill through grantRolePrivs... while Done http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3153 PS33, Line 3153: List<PrincipalPrivilege> removedGrantOptPrivileges = Lists.newArrayList(); > we've been sticking to newArrayList to keep consistency. we could move to a Done http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3171 PS33, Line 3171: addedRolePrivileges = Lists.newArrayList(); > here for consistency Done http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3178 PS33, Line 3178: List<TCatalogObject> updatedPrivs = Lists.newArrayList(); > here for consistency Done http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3183 PS33, Line 3183: List<TCatalogObject> removedPrivs = Lists.newArrayList(); > here for consistency 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: 33 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: Wed, 12 Sep 2018 17:58:03 +0000 Gerrit-HasComments: Yes
