Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33 PS7, Line 33: virtual Status Start() { return Status::OK(); } : : // Stops the AuthzProvider instance. : virtual void Stop() {} nit: if you have DefaultAuthzProvider with dummy implementation of all authz-related methods, why not to make these two methods pure virtual and add the default implementation for these two methods into DefaultAuthzProvider as well? http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@147 PS7, Line 147: Authorize Here and below for negative cases: did you actually mean 'Don't authorize' here? http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@238 PS7, Line 238: TEST_P(SentryAuthzProviderTest, TestReconnect) { What happens if Sentry server is not responsive e.g., due network errors or the server being too busy and the authz provider tries to get authz info from the server? Do you think it's worth adding a small test for that case to ensure the provider behaves appropriately? I think it's pretty easy to simulate a too busy server: just pause it by sending the SIGSTOP signal to the server process. http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@51 PS7, Line 51: virtual nit: here and below drop 'virtual' since that's a derived class and the 'override' is present for the overridden methods. http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@50 PS7, Line 50: sentry.service.client.server.rpc-addresses This seems confusing: ...service.client.server.... Is there any typo in the name of the property? http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56 PS7, Line 56: server_name If that's to describe server namespace in Sentry terms, why not 'sentry_server_namespace? Is that just for compatibility with Impala's --server_name flag? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@218 PS3, Line 218: set<TSentryGroup> groups; > It will return an 'Invalid Input' error. OK, thanks for the info. -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 17:28:23 +0000 Gerrit-HasComments: Yes
