Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11039 )
Change subject: IMPALA-7342: Add initial support for user-level permissions ...................................................................... Patch Set 19: (6 comments) http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java: http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@49 PS18, Line 49: principal > perhaps define this as a PrincipalType, then use it in the remaining places Done http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@59 PS18, Line 59: et the principal ID to probe the pr > couldn't parse this (nor could I before). perhaps replace 'using' with 'pro Done http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@68 PS18, Line 68: > nit: names Done http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@239 PS18, Line 239: > I can see that it makes sense to model the Thrift as an object that contain I thought about doing this initially but I hit a Java limitation of generics since Java generics is invariant. Here's my attempt. class Principal { .. } class Role extends Principal { .. } class User extends Principal { .. } private final CatalogObjectCache<Role> roleCache_ = new CatalogObjectCache<>(); private final CatalogObjectCache<User> userCache_ = new CatalogObjectCache<>(); // need to update the generic parameter to ? extends Principal private CatalogObjectCache<? extends Principal> getPrincipalCache(TPrincipalType type) { return type == TPrincipalType.ROLE ? roleCache_ : userCache_; } public synchronized void addPrincipal(Principal principal) { CatalogObjectCache<? extends Principal> cache = getPrincipalCache(principal.getPrincipalType()); cache.add(principal); // compilation error .... } Another possible option is to keep the caches to use Principal, but when we retrieve an element from the cache, we need to perform a cast. The worst thing for List<Role> getAllRoles or List<User> getAllUsers, we need to iterate every element in of List<Principal> and return an appropriate List<User> or List<Role> since we can't just cast List<Principal> to List<User> or List<Role> since Java's List is invariant. I'm open for a better suggestion. http://gerrit.cloudera.org:8080/#/c/11039/18/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/11039/18/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@449 PS18, Line 449: for (Principal user: getAllUsers()) { : addPrincipalToCatalogDelta(user, ctx); : } > why is this change here? I was expecting a change like this in the patch th It's a way so that we can test if the refactor works. The update in AuthorizationStmtTest requires this change. It is not something that we expose externally yet. We only use it internally. http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2326 PS18, Line 2326: public void create(TPrivilege[]... privileges) throws ImpalaException { > pull these up to single line where applicable. Done. -- To view, visit http://gerrit.cloudera.org:8080/11039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07e0d46d2e50d35bd64ee573b5aa4b779eb9e62f Gerrit-Change-Number: 11039 Gerrit-PatchSet: 19 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Adam Holley <[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, 14 Aug 2018 19:45:04 +0000 Gerrit-HasComments: Yes
