You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/11/10 21:51:24 UTC
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
-----------------------------------------------------------
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.
Changes
-------
Addressed mpark's comments
Summary (updated)
-----------------
Added `execute()` method to process::Subprocess
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 (updated)
-----
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
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Jie Yu <yu...@gmail.com>.
> 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.
>
> Marco Massenzio wrote:
> 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;
> ```
>
> Jie Yu wrote:
> From the code above, can you just caputure commandInfo.command() in the lambda and print it?
>
> ```
> string command = commandInfo.command();
>
> result_.then([command](...) {
> ...
> LOG(INFO) << command << "...";
> });
> ```
ALso, `auto s = process::subprocess(commandInfo.command(), args);` this line fork and exec the subprocess. So having another `s->execute()` sounds very confusing to me.
- 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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Jie Yu <yu...@gmail.com>.
> 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.
>
> Marco Massenzio wrote:
> 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?
I guess I don't understand in what scenario should you issue killtree?
Why it's not reliable? What if a process reparents to init? What if a process calls setsid to escape from a session?
- 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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Jie Yu <yu...@gmail.com>.
> 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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Jie Yu <yu...@gmail.com>.
> 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?
>
> Marco Massenzio wrote:
> 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.
>From what I can tell from the code you pasted below, doesn't seem to simplify the caller's code *a lot*.
- 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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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?
>
> Marco Massenzio wrote:
> 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.
>
> Jie Yu wrote:
> From what I can tell from the code you pasted below, doesn't seem to simplify the caller's code *a lot*.
Point well taken, but I'm confused here, Jie - on the one hand, you here seem to be unhappy it does not simplify enough, then below you suggest it simplifies too much.
The point here is - BenH and I went over the current usages of `subprocess()` and saw a common pattern, and agreed on a way that would enable us to (a) cover 99% of usage patterns and (b) avoid the same boilerplate code to be repeated over and over again.
Three months on, and countless reviews after, this is what we arrived at: it would be good, at some point in time, to accept that this is **one** way of doing things, among countless others - and I'd like this code committed.
Unless we all agree that it's either buggy/flawed/wrong, in which case, I'm happy to discard this patch and re-start from scratch.
> 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).
>
> Jie Yu wrote:
> 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)?
Please see the usage in the tests (this same patch).
`returnCode` is EXIT_FAILURE and `signal` is non-zero - in case one cares (which most folks don't seems to, but whatever).
if we can't get the exit status (`Subprocess::status` is `None()`) we return a `Failure` on the returned future and an error message (which is what ultimately is done practically everywhere `subprocess()` is used - while I guess I could've dreamt up tens of different scenarios, usage is pretty consistent in Mesos codebase and is what we're trying to encapsulate here).
Note that `Subprocess::status` is still available to callers.
> 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.
>
> Marco Massenzio wrote:
> 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;
> ```
>
> Jie Yu wrote:
> From the code above, can you just caputure commandInfo.command() in the lambda and print it?
>
> ```
> string command = commandInfo.command();
>
> result_.then([command](...) {
> ...
> LOG(INFO) << command << "...";
> });
> ```
>
> Jie Yu wrote:
> ALso, `auto s = process::subprocess(commandInfo.command(), args);` this line fork and exec the subprocess. So having another `s->execute()` sounds very confusing to me.
I don't disagree - what would you suggest instead?
(note about the example above: it's contrived - one can also imagine storing the Future in a map keyed by the id, and retrieve the outcome upon receiving a GET requests on the pid status; there may be other scenarios where just passing the commandInfo and/or the args or whatever in the lambda may be less desirable -- but, again, this is *one* way of doing things, not necessarily unique, and admittedly maybe not even *the* best).
> 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.
>
> Marco Massenzio wrote:
> 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?
>
> Jie Yu wrote:
> I guess I don't understand in what scenario should you issue killtree?
>
> Why it's not reliable? What if a process reparents to init? What if a process calls setsid to escape from a session?
well, I'd ratherr have something that works 99.9% of the cases in which it tries to clean up after itself, than nothing - obviously, if the process tries to do something clever to escape / avoid termination, I'm really not sure what else could one do about it?
Again - what is your alternative suggestion?
In the apidoc I was being rather explicit about the "no-guarantees" nature of this method:
```
the caller may want to try and clean up the process.
```
we give no refunds for failures to cleanup :)
(we can of course be even more explicit about the issues you mentioned).
- 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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Michael Park <mc...@gmail.com>.
> 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.
>
> Marco Massenzio wrote:
> 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;
> ```
>
> Jie Yu wrote:
> From the code above, can you just caputure commandInfo.command() in the lambda and print it?
>
> ```
> string command = commandInfo.command();
>
> result_.then([command](...) {
> ...
> LOG(INFO) << command << "...";
> });
> ```
>
> Jie Yu wrote:
> ALso, `auto s = process::subprocess(commandInfo.command(), args);` this line fork and exec the subprocess. So having another `s->execute()` sounds very confusing to me.
>
> Marco Massenzio wrote:
> I don't disagree - what would you suggest instead?
>
> (note about the example above: it's contrived - one can also imagine storing the Future in a map keyed by the id, and retrieve the outcome upon receiving a GET requests on the pid status; there may be other scenarios where just passing the commandInfo and/or the args or whatever in the lambda may be less desirable -- but, again, this is *one* way of doing things, not necessarily unique, and admittedly maybe not even *the* best).
In terms of the name, why not `communicate` given that the behavior is similar to Python's `communicate`?
In terms of whether to store the `Invocation`, I can see how storing it would simplify the caller's code in cases where there are multiple async commands.
The caller would otherwise need to manually pair-up the commands to the corresponding results, by storing them in different data structures in the same order, pairing them up explicitly via `std::pair`, or whatever else.
Having said that, I think it would be great to have an example from our codebase to make a stronger argument for this.
- Michael
-----------------------------------------------------------
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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Jie Yu <yu...@gmail.com>.
> 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.
>
> Marco Massenzio wrote:
> 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;
> ```
>From the code above, can you just caputure commandInfo.command() in the lambda and print it?
```
string command = commandInfo.command();
result_.then([command](...) {
...
LOG(INFO) << command << "...";
});
```
- 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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37336/#review107421
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp (lines 53 - 75)
<https://reviews.apache.org/r/37336/#comment166518>
What's the motivation of storing this? Should the caller already have those information?
3rdparty/libprocess/include/process/subprocess.hpp (lines 114 - 117)
<https://reviews.apache.org/r/37336/#comment166519>
Ditto. I am not convinced that this is strictly necessary.
3rdparty/libprocess/include/process/subprocess.hpp (lines 119 - 130)
<https://reviews.apache.org/r/37336/#comment166520>
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.
3rdparty/libprocess/include/process/subprocess.hpp (line 328)
<https://reviews.apache.org/r/37336/#comment166521>
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.
3rdparty/libprocess/include/process/subprocess.hpp (lines 336 - 343)
<https://reviews.apache.org/r/37336/#comment166522>
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.
3rdparty/libprocess/src/subprocess.cpp (lines 185 - 190)
<https://reviews.apache.org/r/37336/#comment166523>
What if outData is called more than once?
- Jie Yu
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
>
>
Re: Review Request 37336: Added `execute()` method to
process::Subprocess
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37336/#review105960
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37336]
All tests passed.
- Mesos ReviewBot
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
>
>