Alexey Serbin has posted comments on this change. Change subject: Add basic Hive MetaStore client ......................................................................
Patch Set 12: (7 comments) http://gerrit.cloudera.org:8080/#/c/7053/10/cmake_modules/FindJavaHome.cmake File cmake_modules/FindJavaHome.cmake: PS10, Line 55: /usr/java/default > I've changed this to use /usr/libexec/java_home on OS X, which should alway I see -- that's much better. http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/hms_client-test.cc File src/kudu/hms/hms_client-test.cc: PS12, Line 67: LOG(INFO) << "my_db: " << my_db; nit here and below: may be, output the whole info on those objects only if any of the relevant asserts below fail? I'm not a big fan of chatty tests when all is going well. If this chattiness is really useful, that's fine, of course. http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/hms_client.cc File src/kudu/hms/hms_client.cc: PS10, Line 60: > The TException acts as a catchall (in addition to handling network errors). ah, I see. It's safe then. http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/hms_client.cc File src/kudu/hms/hms_client.cc: PS12, Line 49: try { \ : call; \ : } catch (const hive::AlreadyExistsException& e) { \ : return Status::AlreadyPresent(msg, e.what()); nit: not very important here, but anyway it might make sense to add parenthesis around the macro's parameters in the body of the macro. http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: PS12, Line 38: hms_process_ == nullptr nit: the std::unique_ptr has specific operator explicit operator bool() const noexcept; so it might be just CHECK(!hms_process_); http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: PS12, Line 327: Status WaitForBind(pid_t pid, uint16_t* port, const char* kind, MonoDelta timeout) { : // In general, processes do not expose the port they bind to, and : // reimplementing lsof involves parsing a lot of files in /proc/. So, : // requiring lsof for tests and parsing its output seems more : // straight-forward. We call lsof in a loop since it typically takes a long : // time for it to initialize and bind a port. : : string lsof; : RETURN_NOT_OK(GetExecutablePath("lsof", {"/sbin", "/usr/sbin"}, &lsof)); : : const vector<string> cmd = { : lsof, "-wbnP", "-Ffn", : "-p", std::to_string(pid), : "-a", "-i", kind : }; : : MonoTime deadline = MonoTime::Now() + timeout; : string lsof_out; : : for (int64_t i = 1; ; i++) { : lsof_out.clear(); : Status s = Subprocess::Call(cmd, "", &lsof_out); : : if (s.ok()) { : StripTrailingNewline(&lsof_out); : break; : } : if (deadline < MonoTime::Now()) { : return s; : } : : SleepFor(MonoDelta::FromMilliseconds(i * 10)); : } : : // The '-Ffn' flag gets lsof to output something like: : // p19730 : // f123 : // n*:41254 : // The first line is the pid. We ignore it. : // The second line is the file descriptor number. We ignore it. : // The third line has the bind address and port. : vector<string> lines = strings::Split(lsof_out, "\n"); : int32_t p = -1; : if (lines.size() != 3 || : lines[2].substr(0, 3) != "n*:" || : !safe_strto32(lines[2].substr(3), &p) || : p <= 0) { : return Status::RuntimeError("unexpected lsof output", lsof_out); : } : CHECK(p > 0 && p < std::numeric_limits<uint16_t>::max()) << "parsed invalid port: " << p; : VLOG(1) << "Determined bound port: " << p; : *port = p; : return Status::OK(); : } : } // anonymous namespace : : Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) { : return WaitForBind(pid, port, "4TCP", timeout); : } : : Status WaitForUdpBind(pid_t pid, uint16_t* port, MonoDelta timeout) { : return WaitForBind(pid, port, "4UDP", timeout); : } I thought you were about to move this into a separate changelist. http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/util/test_util.h File src/kudu/util/test_util.h: PS12, Line 114: // as well as the $PATH environment variable. : Status GetExecutablePath(const std::string& binary, : const std::vector<std::string>& search, : std::string* path) WARN_UNUSED_RESULT; : : // Waits for the subprocess to bind to any listening TCP port, and returns the port. : Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT; : : // Waits for the subprocess to bind to any listening UDP port, and returns the port. : Status WaitForUdpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT; I thought you were about to move this into a separate changelist. -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
