> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 119-130 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line119> > > > > Why not just use a single `int status` field here. The users can use > > WEXITSTATUS ... to derive those information themself. This is also > > consistent with other places in the code place. > > Marco Massenzio wrote: > `Subprocess` does exactly that (actually, it uses an `Option<int>`) - > this leaves the caller with the burned to do all the legwork: again and again > and again - it's all over the code base. > > The point of this patch is to encapsulate that work, having it done > (hopefully, properly) in **one** place and avoid to have the same code > sprinkled all over the codebase (and you can tell, most places, by copy & > paste).
What if there's no returnCode (due to signal)? Should you use Option<int> returnCode here? Similarly, should you use Option<int> for signal as well. What will the client code be look like? Do you still need to check if (returnCode.isSome()) ... if (signal.isSome()) ... Also 1) what if I want to know if WCOREDUMP is true or not, do you need to add another boolean? 2) what if the reaper cannot reap the subprocess (i.e., we cannot get the exit status)? - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review107421 ----------------------------------------------------------- On Nov. 10, 2015, 8:51 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37336/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2015, 8:51 p.m.) > > > Review request for mesos, Joris Van Remoortere and Michael Park. > > > Bugs: MESOS-3035 > https://issues.apache.org/jira/browse/MESOS-3035 > > > Repository: mesos > > > Description > ------- > > The original API for `process::Subprocess` still left a lot of legwork > to do for the caller; we have now added an `execute()` method > that returns a `Future<Subprocess::Result>`. > > `Subprocess::Result`, also introduced with this patch, contains useful > information > about the command invocation (an `Invocation` struct); the exit code; > `stdout`; > and, optionally, `stderr` too. > > Once the Future completes, if successful, the caller will be able to retrieve > stdout/stderr; whether the command was successful; and whether it received a > signal > > > Diffs > ----- > > 3rdparty/libprocess/include/process/subprocess.hpp > f17816e813d5efce1d3bb1ff1e1111850eeda3ba > 3rdparty/libprocess/src/subprocess.cpp > efe0018d0414c4137fd833c153eb262232e712bc > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > ac600a551fb1a7782ff33cce204b7819497ef54a > > Diff: https://reviews.apache.org/r/37336/diff/ > > > Testing > ------- > > make check > > (also tested functionality with an anonymous module that exposes an > `/execute` endpoint and runs arbitrary commands, asynchronously, > on an Agent) > > > Thanks, > > Marco Massenzio > >
