Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement ...................................................................... Patch Set 14: (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_AUTHORIZATION, /*db*/ null, > nit: Done 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: else { : // Invalidate the entire catalog if no table name is provided. : > Is this needed? Doesn't refreshAuthorization() take care of it? You're right. Those authorization objects are added into the delete logs by CatalogServiceCatalog. Done. 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: /** > Class comment. Done http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/util/SentryProxy.java@75 PS13, Line 75: * This class keeps track of catalogs objects added and removed. > Not sure if this is the right name. Isn't this tracking the Delta, probably Done http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/util/SentryProxy.java@188 PS13, Line 188: y { > Like mentioned below, I think we can get rid of this and handle everything Done. I agree it feels weird to deal with deletion of catalog objects in this method. It's probably a remnant of old code. I updated the code to remove the catalog users and roles in refreshRolePrivileges and refreshUserPrivileges instead. http://gerrit.cloudera.org:8080/#/c/11888/13/fe/src/main/java/org/apache/impala/util/SentryProxy.java@261 PS13, Line 261: } > It is weird that this method is not updating the removed objects.Instead it Done 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 Done -- 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: 14 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: Thu, 03 Jan 2019 22:31:47 +0000 Gerrit-HasComments: Yes
