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 30: (7 comments) Running test builds, will upload latest patch after. 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: > The server name should be exactly the same everywhere. Having it in multipl Agreed but a bigger change than part of this cr. Opened https://issues.apache.org/jira/browse/IMPALA-7553 http://gerrit.cloudera.org:8080/#/c/11314/30/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/30/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java@35 PS30, Line 35: private String serverName_; > We don't support anything that doesn't have the same serverName for everyth Opened https://issues.apache.org/jira/browse/IMPALA-7553 http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java: http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java@34 PS30, Line 34: // Server name needed for privileges. Set during analysis. > There is no way serverName can change in alterTable event - do we really ne We need it here to be able to create privileges in catalogd. Currently catalogd does not configure server_name. http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2664 PS30, Line 2664: if (getAuthzConfig().isEnabled()) return getAuthzConfig().getServerName(); > Style: if statements should always have {} braces and conform to normal Jav Done http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java: http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java@39 PS30, Line 39: // Server name needed for privileges. Set during analysis. > We only support one global serverName. https://issues.apache.org/jira/browse/IMPALA-7553 http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java File fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java: http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java@193 PS30, Line 193: params.setComment(comment_); > Is this an unrelated change? Since the null check was not needed for server_name, I removed this one. http://gerrit.cloudera.org:8080/#/c/11314/30/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/30/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1033 PS30, Line 1033: if (tableName == null) { > This is weird - the method is called updateDatabasePrivileges, but here you 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: 30 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 02:41:19 +0000 Gerrit-HasComments: Yes
