Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8557 )
Change subject: ntp: dump NTP diagnostics when NTP is unsynchronized or error is too high ...................................................................... Patch Set 4: (7 comments) overall looks good, just some nits http://gerrit.cloudera.org:8080/#/c/8557/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8557/4//COMMIT_MSG@7 PS4, Line 7: NTP nit: drop this http://gerrit.cloudera.org:8080/#/c/8557/4//COMMIT_MSG@9 PS4, Line 9: ntp nit: NTP http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/clock/system_ntp.h File src/kudu/clock/system_ntp.h: http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/clock/system_ntp.h@50 PS4, Line 50: log nit: maybe, make this a parameter by default? http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/clock/system_ntp.cc File src/kudu/clock/system_ntp.cc: http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/clock/system_ntp.cc@90 PS4, Line 90: ERROR nit: maybe, just INFO in case of s.ok() ? http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/clock/system_ntp.cc@77 PS4, Line 77: void TryRun(vector<string> cmd, vector<string>* log) { : string exe, out, err; : Status s = FindExecutable(cmd[0], {"/sbin", "/usr/sbin/"}, &exe); : if (!s.ok()) { : LOG_STRING(WARNING, log) << "could not find executable: " << cmd[0]; : return; : } : : cmd[0] = exe; : s = Subprocess::Call(cmd, "", &out, &err); : // Subprocess::Call() returns RuntimeError in the case that the process returns : // a non-zero exit code, but that might still generate useful err. : if (s.ok() || (s.IsRuntimeError() && (!out.empty() || !err.empty()))) { : LOG_STRING(ERROR, log) : << JoinStrings(cmd, " ") : << "\n------------------------------------------" : << (!out.empty() ? Substitute("\nstdout:\n$0", out) : "") : << (!err.empty() ? Substitute("\nstderr:\n$0", err) : "") : << "\n"; : } else { : LOG_STRING(WARNING, log) << "failed to run executable: " << cmd[0]; : } : : } maybe, move this into src/kudu/util/subprocess.cc? http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/util/subprocess.h File src/kudu/util/subprocess.h: http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/util/subprocess.h@220 PS4, Line 220: Status FindExecutable(const std::string& binary, maybe, the better place for that is in path_util.h? http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/8557/4/src/kudu/util/test_util.cc@a307 PS4, Line 307: remove the declaration part from test_util.h as well? -- To view, visit http://gerrit.cloudera.org:8080/8557 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifeb2206a0475b8c8e183a74aee21315e6e43dc33 Gerrit-Change-Number: 8557 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 21 Nov 2017 23:27:52 +0000 Gerrit-HasComments: Yes
