Anonymous Coward #424 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: (10 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: The server name should be exactly the same everywhere. Having it in multiple places just asks for trouble. 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 everything and this serverName is statically configured. Having serverName per entity is just a waste of memory. At a minimum it should be interned. In practice, it is always the string "server1". 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 need it here? 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 Java formatting. That said you don't need an if statement here at all ands can use ? : construct. 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. 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? 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@1023 PS30, Line 1023: * the privilege, it will be removed on the next refresh. ALTER DATABASE SET OWNER > My understanding is that the HMS calls will not return until the Sentry cal HMS calls have a 2-minute timeout after which they return and continue happily 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 handle both database and table privileges? WOuld it be better to be more explicit on whether you are dealing with a database or a table? http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1338 PS30, Line 1338: updateDatabasePrivileges(db.getName(), /* tableName */ null, params.server_name, > No because functions do not have privileges. Why do we call updateDatabasePrivilege here then? http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1849 PS30, Line 1849: private synchronized org.apache.impala.catalog.User addUserIfNotExists( This is really suspicious. There are other writers that call catalog.addUser() which do not go through this code path. I think catalog itself should have addUserIfNotExists() method that is coordinated with all addUsers() calls. -- 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 <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Anonymous Coward #424 Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 11 Sep 2018 00:52:03 +0000 Gerrit-HasComments: Yes
