> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 53-75 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line53> > > > > What's the motivation of storing this? Should the caller already have > > those information?
not necessarily - please note that this is executed asynchronously, so the caller may have several of them running concurrently and having the invocation returned together with the invocation will greatly simplifly the caller's code. The whole point of this patch is to make the caller's API less "low-level" and simplify the life of those using this facility. I have, for example, a module that executes commands upon an HTTP request - having the `invocation` returned to me with the result, greatly simplifies my code and enables me to return a Response without waiting for execution. > On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 114-117 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line114> > > > > Ditto. I am not convinced that this is strictly necessary. Of course it's not *necessary* - but it makes the caller's life a lot easier. This was the whole point of "wrapping" the calls to `subprocess()` with something that avoided the need to do all the legwork > 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. `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). > On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 328 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line328> > > > > I don't like the name 'execute'. When you create the Subprocess > > instance, the subprocss is already launched and exec'ed. This is rather > > waiting for the subprocess to terminate. This method most definitely does **not** wait. This is how one can use it as a caller (code simplified): ``` auto s = process::subprocess(commandInfo.command(), args); if (s.isError()) { LOG(ERROR) << "Could not spawn subprocess: " << s.error(); return http::ServiceUnavailable(s.error()); } store(s.get().pid()); // <-- needed to reconcile with GETs Future<CommandResult> result_ = s->execute(); result_.then([](const Future<CommandResult> &future) { if (future.isFailed()) { // mark the command as failed return Nothing(); } auto result = future.get(); // update status of job - use pid(); something equivalent to: LOG(INFO) << "Result of '" << result.invocation.command << "'was: " << result.stdout(); return Nothing(); }).after(Seconds(30), [s](const Future<Nothing> &future) { // update status of job to timed out; use `invocation` and `stdout`. s.get().cleanup(); return Nothing(); }); http::Response response = http::OK("{\"result\": \"OK\", \"pid\": \"" + stringify(s.get().pid()) + "\"}"); response.headers["Content-Type"] = "application/json"; return response; ``` > On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 336-343 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line336> > > > > Why introduce this method? I think the caller should be responsible for > > killing the subprocess if needed. > > > > Also, os::killtree is in general not reliable and shouldn't be used in > > library code. again, this is all about hiding implementation details from the caller and make their life easier. thanks for heads-up about `killtree()` - what would you suggest we use instead? when you say "unreliable" what do you mean, exactly? that it may fail to actually kill the process or that it risks blowing up the app or segfaulting or whatever? > On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/src/subprocess.cpp, lines 185-190 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122403#file1122403line185> > > > > What if outData is called more than once? Great question! I would guess they'll get an empty string (as I assume that `io::read()` is sitting on an empty buffer) but worth looking into (and testing!) Thanks. - Marco ----------------------------------------------------------- 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 > >
