Bharath Vissapragada 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 5: (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 for every grant/revoke? If so, it is a performance problem? 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: AUTHZ_REFRESH nit: something like AUTHZ_CACHE_INVALIDATION? Also, given this is a 'special' kind of Catalog object, document what it does? http://gerrit.cloudera.org:8080/#/c/13134/5/common/thrift/CatalogObjects.thrift@591 PS5, Line 591: TAuthzRefresh nit: same comment on naming. My point is that "refresh" is already confusing enough in Impala's context. 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? 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: response.result.setRemoved_catalog_objects(authzDelta.getCatalogObjectsRemoved()); Isn't this a no-op? 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: getAllAuthzRefreshes should we assert this of size 1? http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2357 PS5, Line 2357: removeAuthzRefresh do we ever need to remove this? http://gerrit.cloudera.org:8080/#/c/13134/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2362 PS5, Line 2362: authzRefresh.setCatalogVersion(incrementAndGetCatalogVersion()); Is this needed? 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_REFRESH: : removeAuthzRefresh(catalogObject.getAuthz_refresh(), dropCatalogVersion); like I commented elsewhere, do we ever need to drop this? 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? 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? -- 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: 5 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Mon, 29 Apr 2019 21:20:24 +0000 Gerrit-HasComments: Yes
