Re: Review Request 37336: Added `execute()` method to process::Subprocess

[Michael Park]

> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 328
> > 
> >
> > 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 result_ = s->execute();
>   result_.then([](const 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 ) {
> // 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`, 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 
> f17816e813d5efce1d3bb1ff1e850eeda3ba 
>   3rdparty/libprocess/src/subprocess.cpp 
> 

Re: Review Request 37336: Added `execute()` method to process::Subprocess

[Jie Yu]

> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 119-130
> > 
> >
> > 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`) - 
> 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 
returnCode here? Similarly, should you use Option 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`, 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 
> f17816e813d5efce1d3bb1ff1e850eeda3ba 
>   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

[Marco Massenzio]

> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 53-75
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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`) - 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
> > 
> >
> > 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 result_ = s->execute();
  result_.then([](const 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 ) {
// 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
> > 
> >
> > 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
> > 
> >
> > 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

Re: Review Request 37336: Added `execute()` method to process::Subprocess

[Jie Yu]

> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 328
> > 
> >
> > 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 result_ = s->execute();
>   result_.then([](const 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 ) {
> // 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`, 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 
> f17816e813d5efce1d3bb1ff1e850eeda3ba 
>   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

[Jie Yu]

> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 328
> > 
> >
> > 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 result_ = s->execute();
>   result_.then([](const 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 ) {
> // 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`, 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 
> f17816e813d5efce1d3bb1ff1e850eeda3ba 
>   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

[Jie Yu]

> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 53-75
> > 
> >
> > 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`, 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 
> f17816e813d5efce1d3bb1ff1e850eeda3ba 
>   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

[Jie Yu]

> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 336-343
> > 
> >
> > 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`, 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 
> f17816e813d5efce1d3bb1ff1e850eeda3ba 
>   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

[Marco Massenzio]

> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 53-75
> > 
> >
> > 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
> > 
> >
> > 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`) - 
> 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 
> returnCode here? Similarly, should you use Option 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
> > 
> >
> > 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 result_ = s->execute();
>   result_.then([](const Future ) {
> if (future.isFailed()) {
>   // mark the command as failed
>   return Nothing();
> }
> auto 

Re: Review Request 37336: Added `execute()` method to process::Subprocess

[Michael Park]

> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 340
> > 
> >
> > I think this violates our use of `auto` rule. Could you please spell 
> > out the type?
> 
> Marco Massenzio wrote:
> I would disagree here... the type is `Try` 
> 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` 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.

Yes, you are correct that the only part we care about is the `Try`. In the 
future, we'll be able to say `Try result = ...;` exactly in these cases.
As is, however we either have to go with `auto` or 
`Try` and neither of them are ideal.
In the context of Mesos however, we tend to err on the side of 
overcommunicating the type.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37336/#review105544
---


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`, 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 
> f17816e813d5efce1d3bb1ff1e850eeda3ba 
>   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

[Mesos ReviewBot]

---
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`, 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 
> f17816e813d5efce1d3bb1ff1e850eeda3ba 
>   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

[Marco Massenzio]

---
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`, 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 
f17816e813d5efce1d3bb1ff1e850eeda3ba 
  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