Adar Dembo 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) Looks like I missed the party; feel free to address my feedback in your next Sentry patch (rather than a standalone patch). 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 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 stuff to make the code less verbose. 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? 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 Sentry roles were added or deleted, and privileges were granted to or revoked from roles. Yet, this is talking about "granting roles"; what does this mean? 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. 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 to be. Would it be possible to add corresponding 'using' statements to shorten this? http://gerrit.cloudera.org:8080/#/c/11657/5/src/kudu/sentry/sentry_client.cc@159 PS5, Line 159: privilege Nit: privileges (below too) -- 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 Gerrit-Comment-Date: Wed, 17 Oct 2018 03:28:46 +0000 Gerrit-HasComments: Yes
