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: (8 comments) 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@1641 PS33, Line 1641: public User addUserIfNotExists(String owner, Reference<Boolean> existingUser) { > Because one of the calls needs to know if it's an existing user or not. What do you think about returning a Pair? 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); > versionLock_ cannot be upgraded to write lock. Yeah, you are right, it isn't upgradeable, but this is the most common case, so you can do a quick check with read lock first and if it isn't there, get a write lock and do a full thing. I am not sure how important this lock is. 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@2869 PS33, Line 2869: ownerType == PrincipalType.ROLE ? TPrincipalType.ROLE : TPrincipalType.USER); > Not currently. Whar if the new type is added later? This code will silently break then. 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() > removePrivilege is synchronized. It is but you perform multiple operations and other things can intervene in between. Is it safe? 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()); > Because those are distinct objects in the catalog. The question is why are incrementing catalog version twice 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(); > Lists.newArrayList seems to be the pattern. Added size. This was the old pattern because you couldn't use <> in older versions of Java. Now it is rather useless and doesn't allow you to specify initial size. 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(); > we've been sticking to newArrayList to keep consistency. we could move to a The reason Lists.newArrayList() was used in older versions of java was to avoid ugly type casts which now you don't have to do. The more serious reason to use new ArrayList() directly is because it allows you to specify the initial array size that can be better then default. 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()) { > No, the parser should not allow statements without privileges. Would be nice to add a comment and a precondition -- 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: Wed, 12 Sep 2018 21:56:45 +0000 Gerrit-HasComments: Yes
