You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/09/26 18:56:32 UTC
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Hello Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/11524
to review the following change.
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
KUDU-2541: Fill out basic sentry client API
This commit contains more plumbin for the upcoming Sentry integration
(KUDU-428). The Sentry client class is filled out with enough methods to
ensure the Thrift RPC is working correctly, as well as error conversion
from Sentry and Thrifts format to Kudu's Status.
Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
---
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/mini_sentry.cc
M src/kudu/sentry/mini_sentry.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
6 files changed, 276 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/11524/1
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11524 )
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.h
File src/kudu/sentry/sentry_client.h:
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.h@61
PS1, Line 61: hms_address
sentry
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 26 Sep 2018 19:05:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11524 )
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 26 Sep 2018 22:59:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11524 )
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/11524/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11524/1//COMMIT_MSG@9
PS1, Line 9: This commit contains more plumbin for the upcoming Sentry integration
plumbing. Or plumbin'
http://gerrit.cloudera.org:8080/#/c/11524/1//COMMIT_MSG@12
PS1, Line 12: Thrifts
Thrift's
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/mini_sentry.cc@142
PS1, Line 142: Hadoops
Hadoop's
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/mini_sentry.cc@147
PS1, Line 147: has
have
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/mini_sentry.cc@199
PS1, Line 199: static const string kUsers = R"(
Maybe a short comment documenting the effect these values have?
http://gerrit.cloudera.org:8080/#/c/11524/2/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11524/2/src/kudu/sentry/sentry_client-test.cc@53
PS2, Line 53: TEST_F(SentryClientTest, TestCreateDropRole) {
Other simple tests for error conversion:
1. Create a duplicate role. AlreadyPresent.
2. Drop a non-existent role. NotFound.
http://gerrit.cloudera.org:8080/#/c/11524/2/src/kudu/sentry/sentry_client-test.cc@61
PS2, Line 61: ::sentry::TCreateSentryRoleRequest req;
How do you feel about building simple Kudu abstractions for the request/response classes? To avoid having ::sentry stuff leak out into the rest of Kudu?
Looks like we didn't do that for HMS integration though.
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.h
File src/kudu/sentry/sentry_client.h:
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.h@60
PS1, Line 60: proided
provided
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 26 Sep 2018 20:36:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11524 )
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
KUDU-2541: Fill out basic sentry client API
This commit contains more plumbing for the upcoming Sentry integration
(KUDU-428). The Sentry client class is filled out with enough methods to
ensure the Thrift RPC is working correctly, as well as error conversion
from Sentry and Thrift's format to Kudu's Status.
Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Reviewed-on: http://gerrit.cloudera.org:8080/11524
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Hao Hao <ha...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/mini_sentry.cc
M src/kudu/sentry/mini_sentry.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
6 files changed, 299 insertions(+), 9 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Hao Hao: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11524 )
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 26 Sep 2018 22:30:07 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11524
to look at the new patch set (#2).
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
KUDU-2541: Fill out basic sentry client API
This commit contains more plumbin for the upcoming Sentry integration
(KUDU-428). The Sentry client class is filled out with enough methods to
ensure the Thrift RPC is working correctly, as well as error conversion
from Sentry and Thrifts format to Kudu's Status.
Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
---
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/mini_sentry.cc
M src/kudu/sentry/mini_sentry.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
6 files changed, 284 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/11524/2
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11524 )
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/11524/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11524/1//COMMIT_MSG@9
PS1, Line 9: This commit contains more plumbing for the upcoming Sentry integration
> plumbing. Or plumbin'
Done
http://gerrit.cloudera.org:8080/#/c/11524/1//COMMIT_MSG@12
PS1, Line 12: Thrift'
> Thrift's
Done
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/mini_sentry.cc@142
PS1, Line 142: Hadoop'
> Hadoop's
Done
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/mini_sentry.cc@147
PS1, Line 147: hav
> have
Done
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/mini_sentry.cc@199
PS1, Line 199: // Simple file format containing mapping of user to groups in INI syntax, see
> Maybe a short comment documenting the effect these values have?
Done
http://gerrit.cloudera.org:8080/#/c/11524/2/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11524/2/src/kudu/sentry/sentry_client-test.cc@53
PS2, Line 53: TEST_F(SentryClientTest, TestCreateDropRole) {
> Other simple tests for error conversion:
Done
http://gerrit.cloudera.org:8080/#/c/11524/2/src/kudu/sentry/sentry_client-test.cc@61
PS2, Line 61: ::sentry::TCreateSentryRoleRequest req;
> How do you feel about building simple Kudu abstractions for the request/res
Right, the way this works on the HMS side is that we have the base hms-client which exposes the raw types, then a higher level component which provides a nicer API on top of that (hms-catalog). I'm planning to do the same thing for Sentry.
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.h
File src/kudu/sentry/sentry_client.h:
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.h@60
PS1, Line 60: thrift
> provided
Done
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.h@61
PS1, Line 61:
> sentry
Done
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.h@61
PS1, Line 61: ~SentryClient();
> warning: function 'kudu::sentry::SentryClient::SentryClient' has a definiti
Done
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 26 Sep 2018 22:25:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11524 )
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.cc
File src/kudu/sentry/sentry_client.cc:
http://gerrit.cloudera.org:8080/#/c/11524/1/src/kudu/sentry/sentry_client.cc@80
PS1, Line 80: }
> warning: do not use 'else' after 'return' [readability-else-after-return]
Done
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 26 Sep 2018 20:25:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2541: Fill out basic sentry client API
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11524
to look at the new patch set (#3).
Change subject: KUDU-2541: Fill out basic sentry client API
......................................................................
KUDU-2541: Fill out basic sentry client API
This commit contains more plumbing for the upcoming Sentry integration
(KUDU-428). The Sentry client class is filled out with enough methods to
ensure the Thrift RPC is working correctly, as well as error conversion
from Sentry and Thrift's format to Kudu's Status.
Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
---
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/mini_sentry.cc
M src/kudu/sentry/mini_sentry.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
6 files changed, 299 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/11524/3
--
To view, visit http://gerrit.cloudera.org:8080/11524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194
Gerrit-Change-Number: 11524
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot