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 33: (37 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 Done 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@56 PS30, Line 56: serverName_ = analyzer.getServerName(); Here and in all other similar places please consider interning the string (it should be the same string everywhere). http://gerrit.cloudera.org:8080/#/c/11314/33/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/33/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java@56 PS33, Line 56: serverName_ = analyzer.getServerName(); Here and in all other similar places you should consider using string interning since serverName is the same everywhere. http://gerrit.cloudera.org:8080/#/c/11314/33/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/33/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1639 PS33, Line 1639: * can be added for a user. example: owner privileges. Please document locking used. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1641 PS33, Line 1641: public User addUserIfNotExists(String owner, Reference<Boolean> existingUser) { Why do you need Reference here? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1644 PS33, Line 1644: User user = getAuthPolicy().getUser(owner); I guess in most cases you get an existing user - would it make sense to get readlock and upgrade it to write lock if this is a new user? 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@1009 PS33, Line 1009: /** Style: The first sentence of Javadoc is used in special ways - it should be a very short description of what the method is doing. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1025 PS33, Line 1025: private void updateOwnerPrivileges(String databaseName, String tableName, Style: you may consider having multiple user-facing functions which are simpler - e.g. addTableOwnerPrivilege, etc and they can all call your fancy common function. This will help with many call sites where you have to specify which null means what. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1037 PS33, Line 1037: if(oldOwner != null && oldOwner.length() > 0) { Style: it is better to use isEmpty() rather then compare size with 0. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1038 PS33, Line 1038: removePrivilegeFromCatalog(oldOwner, oldOwnerType, filter, resp); Can this fail? 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, Can someone else create the database with the same name at the same time so that you'll drop owner privileges for the wrong database? 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(), Is it possible that another threadd drops the table and creates a new one with the same name and you'll update owner privileges on the wrong table? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1849 PS33, Line 1849: TPrivilege privilege = createDatabaseOwnerPrivilegeFilter(databaseName, serverName); Can we do all of this in constructor? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1862 PS33, Line 1862: privilege.setScope(TPrivilegeScope.DATABASE).setServer_name(serverName) Can we do all of this in constructor? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2868 PS33, Line 2868: Principal owner = catalog_.getAuthPolicy().getPrincipal(ownerString, Is any locking required here? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2869 PS33, Line 2869: ownerType == PrincipalType.ROLE ? TPrincipalType.ROLE : TPrincipalType.USER); Can there be any type that is not USER or ROLE? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2873 PS33, Line 2873: PrincipalPrivilege removedPrivilege = catalog_.getAuthPolicy() Is any locking required here? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2876 PS33, Line 2876: owner.setCatalogVersion(catalog_.incrementAndGetCatalogVersion()); Why are we setting different version for owner and removedPrivilege? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2883 PS33, Line 2883: LOG.error("Error removing privilege: ", e); Is it Ok to eat this error? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2891 PS33, Line 2891: private void addPrivilegeToCatalog(String ownerString, PrincipalType ownerType, Are there any locking considerations for this method? 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<>(); Why do you need Reference here? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2902 PS33, Line 2902: } else { What if there is owner type distinct from USER or ROLE? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2909 PS33, Line 2909: owner.setCatalogVersion(catalog_.incrementAndGetCatalogVersion()); Owner is a shared entity - others may access/modify it as well - do you need any locking/synchronization here? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2914 PS33, Line 2914: LOG.error("Error adding privilege: ", e); Is it Ok to eat this exception? 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, Is any locking needed here? 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(); Just use new ArrayList<>(), or better give an estimated size. Also you allocate it here but may overwrite later 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(); Just use new ArrayList<>() (or better specify the size) 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(); Just use new ArrayList(addedRolePrivileges.size()). Also if you know that it will be an empty list or 1-element list you can avoid allocations at all. 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(); Same as above. http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3195 PS33, Line 3195: } else if (privileges.get(0).isHas_grant_opt()) { can privileges be empty? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3204 PS33, Line 3204: removedPrivs.get(removedPrivs.size() - 1).getCatalog_version()); Looks like removedPrivs can be empty here (e.g. if updatedPrivs isn't empty) http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3232 PS33, Line 3232: if (catalog_.getSentryProxy() == null) return false; Style: use ? : operator http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3233 PS33, Line 3233: return catalog_.getSentryProxy().isObjectOwnershipGrantEnabled(); Do we need any locking here? Can getSentryProxy() become null after the check? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3729 PS33, Line 3729: updateOwnerPrivileges(db.getName(), /* tableName */ null, params.server_name, Is there a chance that we do this for the wrong database in case of a race condition? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java File fe/src/main/java/org/apache/impala/util/SentryPolicyService.java: http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java@481 PS33, Line 481: SentryServiceClient client = new SentryServiceClient(); Use try-as-resource construct to open a client http://gerrit.cloudera.org:8080/#/c/11314/33/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/33/fe/src/main/java/org/apache/impala/util/SentryProxy.java@99 PS33, Line 99: // For some tests, we create a config but may not have a config file. Why do you need config file? http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/util/SentryProxy.java@100 PS33, Line 100: if (sentryConfig.getConfigFile() != null && sentryConfig.getConfigFile().length()>0) { use isEmpty() -- 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 <[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 20:34:03 +0000 Gerrit-HasComments: Yes
