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 7: Code-Review+1 (7 comments) Looks good to me, pending some suggestions. I'm +1'ing it to give other reviewers a chance, feel free to ugprade it to a +2 if no one else has any other comments. http://gerrit.cloudera.org:8080/#/c/13134/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13134/7//COMMIT_MSG@11 PS7, Line 11: AUTHZ_REFRESH update? http://gerrit.cloudera.org:8080/#/c/13134/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/13134/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@132 PS7, Line 132: authorization refresh. not clear what this means. http://gerrit.cloudera.org:8080/#/c/13134/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@134 PS7, Line 134: refresh nit: does it make sense to rename this too? (inline with invalidateAuthzCache() or something) http://gerrit.cloudera.org:8080/#/c/13134/7/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/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@179 PS7, Line 179: plugin_.refreshPoliciesAndTags(); Add some supportability logging, like SentryProxy? Took..xxx ms. http://gerrit.cloudera.org:8080/#/c/13134/7/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/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@212 PS7, Line 212: AuthorizationDelta authzDelta = new AuthorizationDelta(); I think this could use a comment. http://gerrit.cloudera.org:8080/#/c/13134/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@222 PS7, Line 222: AuthorizationDelta authzDelta = refreshAuthorization(false); : response.result.setUpdated_catalog_objects(authzDelta.getCatalogObjectsAdded()); Mention that this is done for usability reasons and read-your-own-writes semantics (consistent with Sentry)? http://gerrit.cloudera.org:8080/#/c/13134/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/13134/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java@551 PS7, Line 551: throw new CatalogException("Authz cache invalidation not found: " + Add a comment why this should always exist? -- 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 18:39:01 +0000 Gerrit-HasComments: Yes
