Alexey Serbin has posted comments on this change.

Change subject: [util] added Subprocess::GetExitStatus()

Patch Set 7:

File src/kudu/util/

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?
File src/kudu/util/

PS9, Line 346: ProcFdDir(&fd_dir), "Unab
> child process has invoked ?

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2b16e2a2a53a01982f816b9ee41cc61fd93d4bf
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to