Adar Dembo has posted comments on this change. Change subject: [util] added Subprocess::GetExitStatus() ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/4648/2/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: Line 191: int exit_status; > As I understand, 'working properly' in that case would be firing the assert I'm concerned about test flakiness. Once the subprocess is created, it could be scheduled ahead of the parent process and finish first. As long as the test still passes, that's OK, but I wasn't sure. http://gerrit.cloudera.org:8080/#/c/4648/3/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: PS3, Line 154: static const vector<string> argv = { "/bin/sh", "-c", "exit 0" }; : Subprocess p("/bin/sh", argv); Nit: combine this? argv is only used in this one place. Line 185: static const vector<string> argv = { "cat" }; Nit: same here, combine in actual Subprocess invocation. -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
