Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add SentryAuthzProvider ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG@7 PS4, Line 7: [sentry] add SentryAuthzProvider Could you add a paragraph explaining the module placement choices? http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h@59 PS4, Line 59: AuthorizeAlterTable Should we wrap new_table in a boost::optional, to convey that it isn't necessary for non-rename alters? http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@56 PS4, Line 56: : virtual Status Start() OVERRIDE; : : virtual void Stop() OVERRIDE; Nit: lower-case override for new code. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@262 PS4, Line 262: Slice database; : Slice table; : RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, &table)); I commented on this before, but this can be scoped to TABLE or DATABASE; it's not necessary for SERVER. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@267 PS4, Line 267: case AuthorizableScope::TABLE: : authorizable->__set_table(table.ToString()); : case AuthorizableScope::DATABASE: : authorizable->__set_db(database.ToString()); Missing break statements here? Or if this is intentional, there's a FALLTHROUGH_INTENDED macro. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h File src/kudu/sentry/sentry-test-base.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h@33 PS4, Line 33: class SentryTestBase : public KuduTest { Seems like you could push the Kerberos boolean parameterization down into this fixture, as well as the SetUp() and TearDown() methods. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h@67 PS4, Line 67: protected: Nit: indent by one character. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry_client-test.cc@60 PS4, Line 60: void SetUp() override { Note that when overriding SetUp()/TearDown() you should make sure to chain to the superclass implementations. For SetUp() it should be the first thing you do; for TearDown(), the last. -- 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: 4 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: Fri, 26 Oct 2018 00:19:50 +0000 Gerrit-HasComments: Yes
