Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13134 )
Change subject: IMPALA-8293 (Part 2): Add support for Ranger cache invalidation ...................................................................... Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/13134/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13134/5//COMMIT_MSG@11 PS5, Line 11: object type called AUTHZ_REFRESH to allow broadcasting messages from : Catalogd to Impalads to update their local Ranger caches. > Can you mention the granularity of invalidation? Is everything refreshed fo TBH, I don't know the granularity of the invalidation since this is the implementation detail of Ranger. The good thing is if Ranger has a performance improvement in their refresh API, Impala will get that for free without knowing how it works. http://gerrit.cloudera.org:8080/#/c/13134/5/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/13134/5/common/thrift/CatalogObjects.thrift@42 PS5, Line 42: // A catalog > nit: something like AUTHZ_CACHE_INVALIDATION? Also, given this is a 'specia Done http://gerrit.cloudera.org:8080/#/c/13134/5/common/thrift/CatalogObjects.thrift@591 PS5, Line 591: ft representa > nit: same comment on naming. My point is that "refresh" is already confusin Done http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@179 PS5, Line 179: plugin_.refreshPoliciesAndTags(); > thread-safe? What happens with authz requests in flight? Yes, this is thread-safe now since this is the proper API from Ranger. It puts the requests into LinkedBlockingQueue http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@225 PS5, Line 225: > Isn't this a no-op? Yeah, I have removed it. Done. http://gerrit.cloudera.org:8080/#/c/13134/5/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/13134/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@689 PS5, Line 689: zCacheInvalidation: > should we assert this of size 1? Initially this will be 0, only an explicit grant or refresh authz/invalidate metadata will make this to have a size 1. http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2357 PS5, Line 2357: > do we ever need to remove this? Yeah this is not needed. Removed. Done. http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2362 PS5, Line 2362: > Is this needed? Removed this method. Done. http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java: http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@371 PS5, Line 371: case AUTHZ_CACHE_INVALIDATION: : removeAuthzCacheInvalidation(catalogObject.getAuthz_cache_invalidation(), > like I commented elsewhere, do we ever need to drop this? Probably not, but this is part removeCatalogObjects. So it's probably a good idea to just implement it instead of throwing an exception. Like we could have a new Impalad catalog implementation that clears everything and this may require every catalog object type to deal with the removal. http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/service/Frontend.java@266 PS5, Line 266: catalogManager_.setAuthzChecker(authzChecker_); : authzManager_ = authzFactory.newAuthorizationManager(catalogManager_, : authzChecker_::get); > Curious if AuthzChecker can be a part of AuthzManager? We don't use AuthorizationChecker in catalogd. So, it's probably good to split the two. http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/test/resources/ranger-hive-security.xml File fe/src/test/resources/ranger-hive-security.xml: http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/test/resources/ranger-hive-security.xml@47 PS5, Line 47: 30000 > seems high, any particular reason to override? This is actually the default polling time in Ranger. We changed it to 5 seconds to reduce the sleep time. Since we no longer need to sleep with this CR, I'm changing it back to 30 seconds. It's also good to test the code to make sure an explicit refresh works. -- To view, visit http://gerrit.cloudera.org:8080/13134 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia7160c082298e0b8cc2742dd3facbd4978581288 Gerrit-Change-Number: 13134 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Austin Nobis <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 30 Apr 2019 00:02:49 +0000 Gerrit-HasComments: Yes
