Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement ...................................................................... Patch Set 12: (7 comments) http://gerrit.cloudera.org:8080/#/c/11888/11/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/11/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@138 PS11, Line 138: Preconditions.checkNotNull(tableName_); > Preconditions.checkNotNull(tableName_) between L137-138 Done http://gerrit.cloudera.org:8080/#/c/11888/8/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/11888/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1140 PS8, Line 1140: blic void refreshAuthorizat > Can we reverse it then. if (sentryProxy == null) return; Done. http://gerrit.cloudera.org:8080/#/c/11888/8/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/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3451 PS8, Line 3451: List<TCatalogObject> added = new ArrayList<>(); > Actually thinking a bit more about this, I still am not convinced. I think Thanks for the explanation, that makes sense. I have updated the code to keep track of list of catalog objects added and removed. http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_authorization.py@546 PS8, Line 546: > Can we also do execute_query_expect_failure(select from functional.alltypes Done http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_authorization.py@555 PS8, Line 555: assert any("select" in x for x in result.data) > execute_query_expect_success() Done http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/11888/8/tests/authorization/test_grant_revoke.py@211 PS8, Line 211: update. : """ : db_name = "test_role_privilege_case_x_" + get_random_id(5) : db_name_upper_case = "TEST_ROLE_PRIVILEGE_CASE_Y_" + get_random_id(5).upper() : db_name_mixed_case = "TesT_Role_PRIVIlege_case_z" + get_random_id(5) : role_name = "test_role_" + get_random_id(5) : try: : self.client.execute("create role {0}".format(role_name)) : self.client.execute("grant all on server to {0}".format(role_name)) : self.client.execute("grant role {0} to group `{1}`".format(role_name, : grp.getgrnam(getuser()).gr_name)) : : self.client.execute("create database " + db_name) : self.client.execute("create database " + db_name_upper_case) : self.client.execute("create database " + db_name_mixed_case) : self.client.execute( : "create table if not exists {0}.test1(i int)".format(db_name)) : self.client.execute("create table if not exists {0}.TEST2(i int)".format(db_name)) : self.client.execute("create table if not exists {0}.Test3(i int)".format(db_name)) : : self.client.execute( : "grant select on table {0}.test1 to {1}".format(db_name, role_name)) : self.client.execute( : "grant select on table {0}.TEST2 to {1}".format(db_name, role_name)) : self.client.execute( : "grant select on table {0}.TesT3 to {1}".format(db_name, role_name)) : self.client.execute("grant all on database {0} to {1}".format(db_name, role_name)) : self.client.execute( : "grant all on database {0} to {1}".format(db_name_upper_case, role_name)) : self.client.execute( : "grant all on database {0} to {1}".format(db_name_mixed_case, role_name)) : result = self.client.execute("show grant role {0}".format(role_name)) : assert any('test1' in x for x in result.data) : assert any('test2' in x for x in result.data) : assert any('test3' in x for x in result.data) > can we fold this into test_sentry_refresh? Yeah this actually looks very similar to test_refresh_authorization. I don't know why I put this test here :( http://gerrit.cloudera.org:8080/#/c/11888/8/tests/common/sentry_cache_test_suite.py File tests/common/sentry_cache_test_suite.py: http://gerrit.cloudera.org:8080/#/c/11888/8/tests/common/sentry_cache_test_suite.py@67 PS8, Line 67: refresh_authorization), > indentation looks weird but I'm assuming it is PEP compatible since the jen 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: 12 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: Wed, 12 Dec 2018 23:45:23 +0000 Gerrit-HasComments: Yes
