Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc File src/kudu/sentry/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@121 PS3, Line 121: SentryAuthzProviderTest Is it worth adding a few scenarios to verify how the provider works if Sentry server is unreachable? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc File src/kudu/sentry/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@121 PS3, Line 121: Status SentryAuthzProvider::AuthorizeAlterTable(const string& table_name, : const string& user, : bool* authorized) nit: consider either setting '*authorized' to 'false' in the beginning or add WARN_UNUSED_RESULT attribute for those methods; that's to avoid mistakes of using the result '*authorized' if methods like this return not-OK prematurely. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@222 PS3, Line 222: Status SentryAuthzProvider::ParseAddresses nit: it would be nice if the order of methods in the .cc file matched the order of those in the .h file. 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: request.__set_principalName("test-user"); I'm curious what would be the response if not setting (or clearing/resetting) this field? -- 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: 3 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Wed, 24 Oct 2018 05:13:20 +0000 Gerrit-HasComments: Yes
