Alexey Serbin has posted comments on this change. Change subject: [util] added Subprocess::GetExitStatus() ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/4648/2/src/kudu/util/pstack_watcher.cc File src/kudu/util/pstack_watcher.cc: PS2, Line 178: RETURN_NOT_OK > For consistency with the rest of the function, should use RETURN_NOT_OK_PRE Done http://gerrit.cloudera.org:8080/#/c/4648/2/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: Line 153: TEST_F(SubprocessTest, TestGetExitStatusExitSuccess) { > Can you use initializer lists to set up argv in the new tests? It's a good idea; sure. Line 191: TEST_F(SubprocessTest, TestGetExitStatusSignaled) { > Does this test work properly if /bin/cat finishes before p.Kill() has a cha As I understand, 'working properly' in that case would be firing the assertion for p.Kill() call due to an error while trying to deliver signal to the target process. Are you concerned about a race and sending SIGKILL to some other process instead? Or it's more about adding additional test for some particular scenario? http://gerrit.cloudera.org:8080/#/c/4648/2/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: Line 526: > AFAICT, the three functions below were just moved, right? Or have they chan Both: I re-ordered them to match the declaration order in the header file and also updated the signature for DoWait() since the way it was written it would not behave consistently if other options besides WNOHANG were passed as parameters. -- To view, visit http://gerrit.cloudera.org:8080/4648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b16e2a2a53a01982f816b9ee41cc61fd93d4bf Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
