Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@13 PS1, Line 13: As adding Kerberos support proved to be trickier than expected this : patch contains several other improvements needed for this to work. : > It'd then be great if we could figure out if we actually need the unique lo Done http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc@375 PS3, Line 375: void SetUp() override { > It looks like the test is failing because it can't determine the user group I believe that's a red herring, the same WARN appeared when the test passed as well. Also I don't think it's possible to do the same as with Ranger, but I can swtich to NullGroupsMapping which will silence the warning and each user appear to belong to 0 groups with no failures in name resolution. http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc@437 PS3, Line 437: TEST_F(MasterRangerTest, TestCreateTableAuthorized) { > Given the eventual goal is to parameterize the entirety of this test, we sh hm I'm not sure it's worth rewriting the tests in ranger_client-test. As you said this file will be rewritten, so these tests might actually go away, just wanted to add some quick tests to verify the integration works properly. http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@342 PS1, Line 342: > If we ever expect to have tests that will use Kerberos and not SSL (or vice Done http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@345 PS1, Line 345: _->CreateSe > I'm fine with this with just a comment. Just want to make sure future reade Done http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/postgres/mini_postgres.h File src/kudu/postgres/mini_postgres.h: http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/postgres/mini_postgres.h@66 PS3, Line 66: CHECK_NE(0, port_); > nit: const ref here too? Done http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc@55 PS3, Line 55: } > In a separate patch, could we maybe loop 'psql' in MiniPostgres::Start() un Ack http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc@251 PS3, Line 251: > nit: "download the list" Done http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h File src/kudu/ranger/mini_ranger_configs.h: http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h@378 PS3, Line 378: const char* kCoreSiteTemplate = R"( > Unused? Done http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h@427 PS3, Line 427: ang > How about making this configurable? Done -- To view, visit http://gerrit.cloudera.org:8080/15601 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549 Gerrit-Change-Number: 15601 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 31 Mar 2020 19:00:55 +0000 Gerrit-HasComments: Yes
