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 32: (6 comments) http://gerrit.cloudera.org:8080/#/c/11314/30/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/11314/30/common/thrift/JniCatalog.thrift@76 PS30, Line 76: > Agreed but a bigger change than part of this cr. Opened https://issues.apa add todo's to remove and reference the jira. http://gerrit.cloudera.org:8080/#/c/11314/32/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11314/32/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1639 PS32, Line 1639: . For example nit: condense to (example: owner pr..). http://gerrit.cloudera.org:8080/#/c/11314/32/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1641 PS32, Line 1641: org.apache.impala.catalog.User no need for fully qualified name here (and below on L1644) http://gerrit.cloudera.org:8080/#/c/11314/32/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/32/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2883 PS32, Line 2883: modifying removing http://gerrit.cloudera.org:8080/#/c/11314/32/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2911 PS32, Line 2911: modifying adding http://gerrit.cloudera.org:8080/#/c/11314/32/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11314/32/fe/src/main/java/org/apache/impala/util/SentryProxy.java@222 PS32, Line 222: if (resetVersions_) { something looks off with this change: prior, this branch was run only when the user already existed. that can be fixed by adding a Reference bool parameter to the method indicating if the user was added or not. the other thing I'll note is that prior, there was less contention for the write lock. since we have multiple ways to add a user, this will compete with all other operations. this may be something to look at again when we stress it. -- 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: 32 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: Tue, 11 Sep 2018 17:50:30 +0000 Gerrit-HasComments: Yes
