Alexey Serbin has posted comments on this change. Change subject: [util] added Subprocess::GetExitStatus() ......................................................................
Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/4648/7/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: Line 436: } > We can CHECK If this really is a programmer error of calling GetExitStatus I don't see what this intricate logic, even if it's proven it's correct, would give us here. As it is in PS8, if called when state_ != kExited, this method will: * in release build: return an IllegalState() error with corresponding message, so the caller can handle the status and see that the call did not come through but the process don't crash in production. * in debug build: call DCHECK() with corresponding message, at which point the program will be terminated abnormally, so a programmer (who is supposedly working with debug build) could see it's a programmatic error in their new code. Is it not enough? Line 556: // as many strings as there were 'fds' in the vector and in that order. > I see, thanks for the update here. I am fine if you still want to have this This CHECK() expresses that the code doesn't expect anything besides those exit statuses, since otherwise the consistency of the code is not guaranteed. As I see it, that's where using CHECK() makes sense. This because if (WIFEXITED(status) || WIFSIGNALED(status)) turned to be false, the rest of the code would become inconsistent, since it expects that the process has exited at this point: it sets state_ to kExited. http://gerrit.cloudera.org:8080/#/c/4648/8/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: PS8, Line 420: from > s/from// ? yep, that's a typo: it should have been 'blocking read from the pipe'. PS8, Line 422: which the child invokes execvp() > when the child is invoked via execvp ? yes, that's a typo: it should should have been 'when the child invokes execvp()'. -- 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: 8 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: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
