Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc File src/kudu/integration-tests/master_authz-itest.cc: http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@155 PS1, Line 155: > nit: Is this needed? Doesn't only the SentryAuthzITestHarness need the HMS Done http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@300 PS1, Line 300: make_optional<const string&>(kAdminUser), cluster, client); : CheckTable(kDatabaseName, kSecondTable, : make_optional<const string&>(kAdminUser), cluster, client); : return Status::OK(); > nit: maybe rename these to SetUpExternalMiniServiceOpts() and SetUpExternal Done http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@920 PS1, Line 920: // in Kudu and the > If you replace this with ASSERT_STR_MATCHES, you can reuse it for ranger to Hao said she already used the upper-case version in systest, so that's why I decided against making them consistent. Thanks for the tip. http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@1031 PS1, Line 1031: { > Remove Done http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@1151 PS1, Line 1151: > We may want to consider using testing::Combine on some kSentry/kRanger enum How do you feel about adding a TODO to refactor it once we support C++14? With C++14 we could templatize this. http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc File src/kudu/integration-tests/master_authz-itest.cc: http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc@645 PS3, Line 645: > I'm a bit surprised that this compiles. Maybe I'm missing something, but th Done http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc@689 PS3, Line 689: Status StopAuthzProvider() { : return harness_.StopAuthzProvider(cluster_); : } > Can we not refactor a bit to have the first argument be a MasterAuthzITestH I think this could be done, but that would require us to rewrite the methods in the harness to expect a pointer to the cluster as an argument instead of the client and create their own client which feels weird. How about rewriting this part as well when we can templatize variables? (C++14) http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@54 PS3, Line 54: // Policy key used for searching policies_ > nit: could you mention what the fields are? Done http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@59 PS3, Line 59: // The AuthorizationPolicy contains a set of privileges on a resource to one or : // more users. 'items' is a vector of user-list of actions pair. This struct can : // be used to create new Ranger policies in tests. > nit: should mention the policy name will be based on its contents. Done http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@191 PS3, Line 191: cies since s > nit: "resources"? Done http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@192 PS3, Line 192: used fo > nit: Ranger Done -- To view, visit http://gerrit.cloudera.org:8080/15681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf Gerrit-Change-Number: 15681 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Apr 2020 20:05:28 +0000 Gerrit-HasComments: Yes
