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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
