> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 91 > > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line91> > > > > 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`?
I probably misunderstood your earlier comment. Moved one level up, into `Subprocess`. > On Nov. 7, 2015, 1:06 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 278 > > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line278> > > > > We tend to not use `getX()` style naming for accessors. How about > > `outData()`? great point (wasn't sure, as it's not spelled out in our style guide - and it's not truly a "getter" in the Google sytle sense: `foo_` -> `foo()`) also renamed `stderr_` to `errData` and `getStderr()` to `errData()` - please let me know if that was "overreach"! > On Nov. 7, 2015, 1:06 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 340 > > <https://reviews.apache.org/r/37336/diff/10/?file=1117802#file1117802line340> > > > > I think this violates our use of `auto` rule. Could you please spell > > out the type? I would disagree here... the type is `Try<std::list<os::ProcessTree>>` and adding that, I don't think makes the code any more readable (on the contrary). In fact, we don't really make use of any of that - all we care is that no error is returned: it could very well be a `Try<Pizza>` for all we care :) This seems to me a poster child of the use-case for `auto`... anyways, no biggie, I've added the type. > On Nov. 7, 2015, 1:06 a.m., Michael Park wrote: > > 3rdparty/libprocess/src/subprocess.cpp, lines 202-203 > > <https://reviews.apache.org/r/37336/diff/10/?file=1117803#file1117803line202> > > > > 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(); }); > > ``` I don't quite remember what it was that I came across originally when I tried the above (sans Promise) then Joris suggested the pattern that you see here now, and this works. IIRC I could never get the `onFailed` to get invoked for failed commands, so the pending process never got cleaned up. If you feel strongly this should not be here (and/or are confident the pattern above works) I'm happy to have another go at it. - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review105544 ----------------------------------------------------------- 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 > >
