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 35: (9 comments) thx for the changes. I think the somewhat new issue here is that we're updating catalogd's replica of two sources (sentry and hms). ordinary path is to update hms and let sentry push. 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: } > The catalog should prevent creating the database with the same name at the It does so via that metastoreDdlLock_ afaict. However, updating privs here is outside of that lock, which is held on L1312. The pattern in that block is: 1) do hms op, 2) do local catalog op. Just wondering why these privs are not also updated under that lock? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1837 PS33, Line 1837: } > I don't think this will happen because these calls are in catalogd. create there are possibly multiple impalads, each with multiple, uncoordinated requests. also, these calls are synchronously returned to the impalad... there is no wait for all catalogs to be sync'd unless sync_ddl is used (but we can't assume that). if a sleep was put before this line, could we see that sequence: this thread creates the table and sleeps, another thread drops the table and creates a new one with the same name, then this thread updates owner privs to the owner of the original request which may differ from the more recently created table. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2868 PS33, Line 2868: try { > Shouldn't be required since it's accessing CatalogObjectCache which is supp true, but there are two operations on that cache: 1) get L2868 and 2) mutate L2873. what happens with operations that are interleaved between these two? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2896 PS33, Line 2896: PrincipalPrivilege cPrivilege; > This code has changed slightly in the latest patch. The reference for exis this was my suggestion. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3145 PS33, Line 3145: /** > I think the versionLock_ is sufficient. fwict, that lock is acquired when you drill through grantRolePrivs... while that method seems atomic, its not. there is a loop there and each iter in the loop gets that lock. that seems to be an existing issue, unrelated to this change. were it atomic, then the rest of this method is not mutating these objects but just filling in the response. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3153 PS33, Line 3153: verifySentryServiceEnabled(); > Lists.newArrayList seems to be the pattern. Added size. we've been sticking to newArrayList to keep consistency. we could move to a different style, but lets do that in a separate pass and lets not introduce inconsistent styles in the same file. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3171 PS33, Line 3171: // addedRolePrivileges parameter will contain a list of new privileges without the > Lists.newArrayList seems to be the pattern. Added size. here for consistency http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3178 PS33, Line 3178: .revokeRolePrivileges(requestingUser, roleName, privileges, > Lists.newArrayList seems to be the pattern. Added size. here for consistency http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3183 PS33, Line 3183: List<TCatalogObject> updatedPrivs = > Lists.newArrayList seems to be the pattern. Added size. here for consistency -- 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: 35 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:42:39 +0000 Gerrit-HasComments: Yes
