Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )
Change subject: IMPALA-8281 Sentry frontend decoupling ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/13284/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13284/5//COMMIT_MSG@7 PS5, Line 7: IMPALA-8281 nit add ":" after the JIRA number and maybe just say "Misc Sentry decoupling clean up" http://gerrit.cloudera.org:8080/#/c/13284/5//COMMIT_MSG@9 PS5, Line 9: This patch moves Sentry specific code from the Frontend into the : SentryImpaladAuthorizationManager. this description is only partially correct. I think we should reword it to "This patch moves Sentry specific code to Sentry specific plugin implementation". http://gerrit.cloudera.org:8080/#/c/13284/5//COMMIT_MSG@12 PS5, Line 12: Testing: you added a new test, right? can you update the commit message? http://gerrit.cloudera.org:8080/#/c/13284/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/13284/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@142 PS5, Line 142: existl typo: exist http://gerrit.cloudera.org:8080/#/c/13284/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@162 PS5, Line 162: existl typo: exist http://gerrit.cloudera.org:8080/#/c/13284/5/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/13284/5/tests/authorization/test_grant_revoke.py@384 PS5, Line 384: @pytest.mark.execute_serially : @SentryCacheTestSuite.with_args( : impalad_args="--server_name=server1", : catalogd_args="--sentry_config=" + SENTRY_CONFIG_FILE, : sentry_config=SENTRY_CONFIG_FILE) : def test_grant_revoke_invalid_role(self, unique_name): : role_name = "foobar" : try: : # This will create "foobar" role catalog object. : 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 {0}".format(unique_name)) : : ex = self.execute_query_expect_failure( : self.client, "grant all on database {0} to role non_role".format(unique_name)) : assert "Role 'non_role' does not exist" in str(ex) : : ex = self.execute_query_expect_failure( : self.client, "revoke all on database {0} from role non_role".format(unique_name)) : assert "Role 'non_role' does not exist" in str(ex) : : ex = self.execute_query_expect_failure(self.client, "show grant role non_role") : assert "Role 'non_role' does not exist" in str(ex) : finally: : self.client.execute("drop database {0}".format(unique_name)) : self.client.execute("drop role {0}".format(role_name)) we should move this test to test_sentry.py instead. It doesn't catch the typo error I pointed out so i think we need to have more coverage on this :) - drop role non_role - show grant role non_role - grant role non_role to group -- To view, visit http://gerrit.cloudera.org:8080/13284 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539 Gerrit-Change-Number: 13284 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Thu, 09 May 2019 21:26:15 +0000 Gerrit-HasComments: Yes
