Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation ...................................................................... Patch Set 6: (6 comments) Haven't looked into it very deeply but like other reviewers suggested, we should probably rethink the class design first before we go into the specifics. I made a suggestion, let me know if that doesn't work for some reason. http://gerrit.cloudera.org:8080/#/c/12748/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12748/6//COMMIT_MSG@12 PS6, Line 12: beter typo http://gerrit.cloudera.org:8080/#/c/12748/6/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/12748/6/common/thrift/CatalogObjects.thrift@590 PS6, Line 590: THrift > typo: Thrift I think we should rename it to reflect the intent. Like Todd said, it feels like cache and not an invalidation marker. http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java: http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java@27 PS6, Line 27: public interface AuthorizationRefresher { High level comment. I think the class design is confusing. Does it make sense to club this with the AuthzMgr? I'd assume that a given AuthzMgr [Sentry|Ranger][Catalogd|Impalad]Mgr should be able to have overrides for refresh(). That way we have a single AuthzMgr() that all the callers can talk to? Does that not work for some reason? http://gerrit.cloudera.org:8080/#/c/12748/6/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/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@133 PS6, Line 133: authzRefresher_.refresh(false); > I'm surprised that you don't need to return the CatalogDelta from this meth Should this also be set in the response object too so that the DDL affects are applied on the coordinators right away? (Probably Todd meant the same thing and if so, I have the same question) http://gerrit.cloudera.org:8080/#/c/12748/6/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/12748/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@321 PS6, Line 321: public void setAuthzRefresher(AuthorizationRefresher authzRefresher) { : Preconditions.checkNotNull(authzRefresher); : authzRefresher_ = authzRefresher; : } Like I mentioned elsewhere, if we club all this into the AuthzMgr, we can probably get rid of all AuthzRefresh stuff and it is transparently handled by the AuthzMgr for that role. http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2297 PS6, Line 2297: addAuthzCache > Yea I'm finding this logic somewhat hard to follow here as well. Maybe we c +1 -- To view, visit http://gerrit.cloudera.org:8080/12748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7 Gerrit-Change-Number: 12748 Gerrit-PatchSet: 6 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-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 28 Mar 2019 23:00:58 +0000 Gerrit-HasComments: Yes
