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 33: (36 comments) 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 ( Done 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 inter Done 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. versionLock_ is documented at the top of the file. Is there something additional you are looking for? 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? Because one of the calls needs to know if it's an existing user or not. 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 versionLock_ cannot be upgraded to write lock. 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 Done 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 sim This was originally multiple functions and a previous review suggested to consolidate. 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. Done 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? Yes. However, there isn't currently an easy way to notify the user of any error for these privilege updates. Throwing an exception would probably cause the other metadata catalog updates to fail even though the HMS updates were successful. 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 The catalog should prevent creating the database with the same name at the same time. Any exceptions thrown occur before the privilege update calls. 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 w I don't think this will happen because these calls are in catalogd. creates and drops are initiated by impalad. The create on the new thread won't be allowed to happen until the drop table completes in the catalog, returns to impalad, and all catalogs are synced. 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? I think moving this to a thrift constructor, if possible, would add unnecessary complexity 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? I think moving this to a thrift constructor, if possible, would add unnecessary complexity 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? Shouldn't be required since it's accessing CatalogObjectCache which is supposed to be a thread safe cache. 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? Not currently. 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? removePrivilege is synchronized. 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? Because those are distinct objects in the catalog. 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? Yes. If we threw an exception here, it would just be eaten at a high level. 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? not specifically 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? This code has changed slightly in the latest patch. The reference for existingUser is set from the addUserIfNotExists method. If it's an existing user, we need to increment the version. (next patch will have the correct condition) 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? Done 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 nee That should be the function of the catalog. The owner in these methods should have a different version. The catalog update should resolve. 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? Yes. If we throw and exception here, it would get swallowed at a higher level. 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? I think the versionLock_ is sufficient. 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. Lists.newArrayList seems to be the pattern. Added size. 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) Lists.newArrayList seems to be the pattern. Added 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()). Lists.newArrayList seems to be the pattern. Added size. 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. Lists.newArrayList seems to be the pattern. Added size. 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? No, the parser should not allow statements without privileges. 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 Done 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 Done 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 che No. 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 We should be protected from it given the separation from impalad and catalogd. 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 Done 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? If the config is enabled but there's no config file, we can't get the objectOwnershipConfigValue_. 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() 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: 33 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 02:52:44 +0000 Gerrit-HasComments: Yes
