Todd Lipcon has posted comments on this change. Change subject: subprocess: add ScopedSubprocess ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/6741/1/src/kudu/integration-tests/full_stack-insert-scan-test.cc File src/kudu/integration-tests/full_stack-insert-scan-test.cc: PS1, Line 232: string cmd = Substitute("perf record --pid=$0", getpid()); : if (FLAGS_perf_fp_flag) cmd += " --call-graph fp"; I think the original code here intended that you should pass --perf_fp_flag on the systems that require the 'fp' parameter, but that itwould _always_ capture the call graph. http://gerrit.cloudera.org:8080/#/c/6741/1/src/kudu/util/subprocess.h File src/kudu/util/subprocess.h: Line 175: // Subprocess that is automatically killed when it goes out of scope. I'm not entirely following this, cuz Subprocess itself already does automatically kill itself when going out of scope, doesn't it? The difference here only seems to be that the signal can be set, and it waits for the exit code, rather than the default which does -9? One thought: what if you move the 'kill and wait' into a member function of subprocess itself, and then it would be reasonably easy to just use a MakeScopedCleanup([&]() { p.KillAndWait(); } or whatever instead of this RAII helper? Line 185: ~ScopedSubprocess() { nit: given this is a pretty heavy-weight function, i think it's better to define out of line in the .cc Line 190: WARN_NOT_OK(p_.Kill(signum_), "Could not kill subprocess"); would be useful to include argv[0] or something in these warning messages here and below Line 191: Status s = p_.Wait(); should there be any kind of timeout and fallback to kill -9? -- To view, visit http://gerrit.cloudera.org:8080/6741 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf31bd4ea6de2917521a9852714fb33cfbec1f61 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
