----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review105544 -----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp (line 42) <https://reviews.apache.org/r/37336/#comment164159> Remove extra newline. 3rdparty/libprocess/include/process/subprocess.hpp (line 91) <https://reviews.apache.org/r/37336/#comment164161> How come `Invocation` is within `Subprocess::Result`? Wouldn't it make more sense for it to be at the `Subprocess` level to be `Subprocess::Invocation`? 3rdparty/libprocess/include/process/subprocess.hpp (line 110) <https://reviews.apache.org/r/37336/#comment164160> Remove extra line. 3rdparty/libprocess/include/process/subprocess.hpp (line 112) <https://reviews.apache.org/r/37336/#comment164162> `s/invokedWith/invocation/`? 3rdparty/libprocess/include/process/subprocess.hpp (line 140) <https://reviews.apache.org/r/37336/#comment164168> It seems like we already chose `out` as the replacement to `stdout` and it refers to file descriptors. This makes me think that we should probably call this `outData` or something instead which communicates more clearly that it's the coming out of `stdout`. 3rdparty/libprocess/include/process/subprocess.hpp (line 278) <https://reviews.apache.org/r/37336/#comment164166> We tend to not use `getX()` style naming for accessors. How about `outData()`? 3rdparty/libprocess/include/process/subprocess.hpp (line 340) <https://reviews.apache.org/r/37336/#comment164163> I think this violates our use of `auto` rule. Could you please spell out the type? 3rdparty/libprocess/include/process/subprocess.hpp (line 389) <https://reviews.apache.org/r/37336/#comment164164> `s/invokedWith_/invocation_/`? 3rdparty/libprocess/src/subprocess.cpp (lines 202 - 203) <https://reviews.apache.org/r/37336/#comment164175> Why do we need the `promise` stuff here? Does something like the following not work? ```cpp Future<tuple<Future<Option<int>>, Future<string>, Future<string>>> result = await(status(), getStdout(), getStderr()); return result.then([=](...) { ... }) .onFailed([=](...) { ... }) .onDiscard([this]() { cleanup(); }); ``` - Michael Park On Nov. 6, 2015, 6:24 a.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37336/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2015, 6:24 a.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 > >
