Dinesh Bhat has posted comments on this change.

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


Patch Set 9:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4648/7/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

Line 436:           }
> I don't see what this intricate logic, even if it's proven it's correct, wo
What I meant by programmatic error was preventing callsites like this:

Subprocess p;
p.call();
p.exitstatus(); // "Subprocess not yet exited" would be misleading
                // because child may have exited, but we didn't issue wait.

Is there an easier way to catch it ? Not sure. Please note that as it is 
currently, DCHECK might misfire if Wait returned as Timedout in L553(in PS7 
diff). So if you want to keep both debug and release bits with error returning 
as it was earlier, that's fine too.


http://gerrit.cloudera.org:8080/#/c/4648/9/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

PS9, Line 346: child has process invoked
child process has invoked ?


Line 439:         close(sync_pipe[0]);
PCHECK for close ?


Line 443:         } else if (rc == -1) {
I am wondering if this can be clubbed at L431.


Line 451:     }
Brainstorming with you another idea here(not suggesting to include in this 
patch) :  If we had clubbed suprocess.call() and subprocess.Wait(BLOCKING) in 
one routine, we need not have this trickery for top level blocking callers. 
i.e, callers of subprocess.call() can expect the routine to block until the 
child finishes execution - my guess is that python subprocess library probably 
has one flavor of such implementation.


-- 
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: 9
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

Reply via email to