Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement ...................................................................... Patch Set 13: (7 comments) http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@110 PS13, Line 110: return new ResetMetadataStmt(Action.REFRESH_FUNCTIONS, db, /*table*/ null, nit: return new Reset..Stmt(Action.REFRESH_FUNCTIONS, Preconditions.checkNotNull(db), null, null) (multiple places). http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3457 PS13, Line 3457: for (TCatalogObject obj: removed) { : catalog_.getDeleteLog().addRemovedObject(obj); : Is this needed? Doesn't refreshAuthorization() take care of it? http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/util/SentryProxy.java@74 PS13, Line 74: @VisibleForTesting Class comment. http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/util/SentryProxy.java@75 PS13, Line 75: protected static class AuthorizationCatalog { Not sure if this is the right name. Isn't this tracking the Delta, probably AuthorizationDelta or something along those lines? http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/util/SentryProxy.java@188 PS13, Line 188: rolesToRemove Like mentioned below, I think we can get rid of this and handle everything using AuthzDelta... http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/util/SentryProxy.java@261 PS13, Line 261: authzCatalog.getAddedCatalogObjects().add(role.toTCatalogObject()); It is weird that this method is not updating the removed objects.Instead it is doing it above in the caller. Same with Users. http://gerrit.cloudera.org:8080/#/c/11888/13/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/11888/13/tests/authorization/test_authorization.py@517 PS13, Line 517: @CustomClusterTestSuite.with_args( Should we add some test coverage for sync_ddl? I'd assume that is a common use case from customers. IMO following combinations should be tested. sync_ddl in {true, false} - Add X roles drop Y roles - Add X roles, drop nothing - Drop Y roles, add nothing (if sync_ddl is true, make sure the changes are propagated in a different coordinator) -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Fri, 21 Dec 2018 20:34:30 +0000 Gerrit-HasComments: Yes
