Alexey Serbin has posted comments on this change. Change subject: [util] added Subprocess::GetExitStatus() ......................................................................
Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/4648/7/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: Line 436: if (state_ != kExited) { > What I meant by programmatic error was preventing callsites like this: ok, I see. I agree, the error message could be misleading -- I'll address that. As for WaitNoBlock() and Timedout: if Subprocess::DoWait() returned Status::Timedout() error, then calling GetExitStatus() is a programmatic error, isn't it? http://gerrit.cloudera.org:8080/#/c/4648/9/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: PS9, Line 346: ProcFdDir(&fd_dir), "Unab > child process has invoked ? Done Line 439: string info; > PCHECK for close ? I thought it might be something bad with the pipe at this point in case of an error, in which case it would be just best effort attempt. However, after some contemplation, I think it's better to add PCHECK(). Done. Line 443: if (status == 0) { > I am wondering if this can be clubbed at L431. It could be, but then the code would be less readable: ti would be calls to close() in multiple places for creating a wrapper to close the read side of the pipe. The current code structuring is intentional: by my opinion it has better readability and better structure than the version with clubbed version. Line 451: status = WTERMSIG(wait_status_); > Brainstorming with you another idea here(not suggesting to include in this I think it's a good idea. It would be nice to have that simpler interface. And probably, we could try to make the callers to use a new paradigm while working with sub-processes. However, due to current use-cases in the field, for some callers it would mean to address the necessity to spawn a separate thread (e.g., take a look at the mini cluster). So, there is more to address in this context. Let's consider it as another opportunity to do refactoring of this code. -- 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: 7 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
