Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11657 )
Change subject: [sentry] Fill out more sentry client API ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client-test.cc@181 PS5, Line 181: TestListPrivilege > Nit: TestListPrivileges Done http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client-test.cc@183 PS5, Line 183: ::sentry::TSentryAuthorizable authorizable; > Like sentry_client.cc, it would be nice to add "using" statements for this Done http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client-test.cc@190 PS5, Line 190: request.__set_principalName("viewer"); > Just curious, but why is this API prefixed by two underscores? Hmm, this is an auto generated API by thrift compiler, though from a quick search it seems thrift chose to use it to avoid conflict with the names of other user-defined fields (https://issues.apache.org/jira/browse/THRIFT-627?focusedCommentId=12969020&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-12969020). http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client-test.cc@211 PS5, Line 211: // Similar to above test to verify that the client can communicate with the : // Sentry service to grant roles, and errors are converted to Status : // instances. > I'm finding this comment (and the test name) a little strange: I thought in Sorry I should make it more clear. This is to grant role to groups, so that users from the groups can have the privilege assigned to the role. I will update the comment. http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client.h File src/kudu/sentry/sentry_client.h: http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client.h@103 PS5, Line 103: private: > Nit: this should be indented by one character. Done http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client.cc File src/kudu/sentry/sentry_client.cc: http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client.cc@156 PS5, Line 156: const ::sentry::TListSentryPrivilegesRequest& request, : ::sentry::TListSentryPrivilegesResponse* response) > The ::sentry:: prefixes are making the code here more verbose than it needs Done by Dan's commit https://gerrit.cloudera.org/#/c/11712/ http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client.cc@159 PS5, Line 159: privilege > Nit: privileges Done -- To view, visit http://gerrit.cloudera.org:8080/11657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34695cd4cc6723b70617164d58f8681cefd09ddd Gerrit-Change-Number: 11657 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[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-Comment-Date: Mon, 22 Oct 2018 18:41:01 +0000 Gerrit-HasComments: Yes
