Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/CMakeLists.txt@91 PS3, Line 91: ADD_KUDU_TEST(master_sentry-itest RUN_SERIAL true PROCESSORS 4) > How did you arrive at the PROCESSORS value? And why use RUN_SERIAL here? I think this is similar to the test in master_hms-itest, so I used its config as a reference. http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@55 PS3, Line 55: const ExternalMiniCluster::BindMode bind_mode; > Looks like you're not using this param in SetUp(). Nor do I think you need Done http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@71 PS3, Line 71: opts.enable_sentry = true; > What happens if enable_sentry=true but enable_kerberos=false? The MiniSentr Yes. In the follow up patch I removed the situation when enable_kerberos=false since it doesn't make much sense for Sentry integration test. http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@74 PS3, Line 74: opts.num_masters = 1; : opts.num_tablet_servers = 1; > These are the defaults; you can omit this. Done http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@93 PS3, Line 93: TestFoo > Rename to something more useful. I revised this test to have a meaningful test in https://gerrit.cloudera.org/#/c/11797/3/src/kudu/integration-tests/master_sentry-itest.cc http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@111 PS3, Line 111: exist > Nit: exists Done http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@116 PS3, Line 116: const auto& params = GetParam(); > Only used in one place below, so just inline GetParam(). Done http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/mini-cluster/mini_cluster.cc@52 PS3, Line 52: int idx; : switch (type) { : case MASTER: : idx = kServersMaxNum - index; : break; : case TSERVER: : idx = index + 1; : break; : case EXTERNAL_SERVER: : idx = kServersMaxNum / 3 + index; : break; > Not really your fault, but could you add a comment here walking through wha Done http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@a421 PS3, Line 421: : > This structure of the pairs is lost in the new comment. Mind keeping it aro Done http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@402 PS3, Line 402: // The '-Ffn' flag gets lsof to output something like: : // p5801 : // f548 : // n127.0.0.1:43954->127.0.0.1:43617 : // f549 : // n*:8038 > Hrm, I think I'm missing something. The command didn't change, right? Why d The output format didn't change, but the way we parse the output is only good for wildcard binding (n*). http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@425 PS3, Line 425: Status s = Subprocess::Call(cmd, "", &lsof_out).AndThen([&] () { : StripTrailingNewline(&lsof_out); : vector<string> lines = strings::Split(lsof_out, "\n"); : for (int index = 2; index < lines.size(); index += 2) { : StringPiece cur_line(lines[index]); : if (HasPrefixString(cur_line.ToString(), addr_pattern) && : !cur_line.contains("->")) { : if (!safe_strto32(lines[index].substr(addr_pattern.size()), &p)) { : return Status::RuntimeError("unexpected lsof output", lsof_out); : } : : return Status::OK(); : } : } : : return Status::RuntimeError("unexpected lsof output", lsof_out); : }); > nit: merge these `if`s into one conditional block? Also mind adding a comme The explanation is added in the previous comment section, which includes the new behavior. Or maybe you think that is not enough? I don't see how we can merge the if's block? Any suggestions? -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 10 Dec 2018 07:13:23 +0000 Gerrit-HasComments: Yes
