Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

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

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.


> On Nov. 7, 2015, 1:06 a.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 202-203
> > 
> >
> > Why do we need the `promise` stuff here?
> > 
> > Does something like the following not work?
> > 
> > ```cpp
> > Future, Future, Future>> 
> > 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`, 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: Simplified the caller interface to process::Subprocess

[Michael Park]

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



3rdparty/libprocess/include/process/subprocess.hpp (line 42)


Remove extra newline.



3rdparty/libprocess/include/process/subprocess.hpp (line 91)


How come `Invocation` is within `Subprocess::Result`? Wouldn't it make more 
sense for it to be at the `Subprocess` level to be `Subprocess::Invocation`?



3rdparty/libprocess/include/process/subprocess.hpp (line 110)


Remove extra line.



3rdparty/libprocess/include/process/subprocess.hpp (line 112)


`s/invokedWith/invocation/`?



3rdparty/libprocess/include/process/subprocess.hpp (line 140)


It seems like we already chose `out` as the replacement to `stdout` and it 
refers to file descriptors. This makes me think that we should probably call 
this `outData` or something instead which communicates more clearly that it's 
the coming out of `stdout`.



3rdparty/libprocess/include/process/subprocess.hpp (line 278)


We tend to not use `getX()` style naming for accessors. How about 
`outData()`?



3rdparty/libprocess/include/process/subprocess.hpp (line 340)


I think this violates our use of `auto` rule. Could you please spell out 
the type?



3rdparty/libprocess/include/process/subprocess.hpp (line 389)


`s/invokedWith_/invocation_/`?



3rdparty/libprocess/src/subprocess.cpp (lines 202 - 203)


Why do we need the `promise` stuff here?

Does something like the following not work?

```cpp
Future, Future, Future>> result =
  await(status(), getStdout(), getStderr());

return result.then([=](...) { ... })
 .onFailed([=](...) { ... })
 .onDiscard([this]() { cleanup(); });
```


- Michael Park


On Nov. 6, 2015, 6:24 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Nov. 6, 2015, 6:24 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The original API for `process::Subprocess` still left a lot of legwork
> to do for the caller; we have now added an `execute()` method
> that returns a `Future`.
>  
> `Subprocess::Result`, 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: Simplified the caller interface to process::Subprocess

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [37336]

Failed command: ./support/apply-review.sh -n -r 37336

Error:
 2015-11-07 03:43:56 URL:https://reviews.apache.org/r/37336/diff/raw/ 
[16081/16081] -> "37336.patch" [1]
error: patch failed: 3rdparty/libprocess/include/process/subprocess.hpp:27
error: 3rdparty/libprocess/include/process/subprocess.hpp: patch does not apply
error: patch failed: 3rdparty/libprocess/src/tests/subprocess_tests.cpp:40
error: 3rdparty/libprocess/src/tests/subprocess_tests.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


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`, 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: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 56
> > 
> >
> > Could you explain in what situations and why the list of arguments 
> > could/would be modified...?

I think I should remove this comment (in fact, I just did): originally, I was 
hoping to be able to somehow fix the awkward situation in which `arg[0]` is 
just the `progname` and not actually used as a command argument: while this is 
"obvious" for any C/C++ developer, it is also undocumented in `os::shell` and 
leads to unexpected behavior for unaware users of `CommandInfo` (for example).

It turns out that that's not possible without breaking a lot of existing 
code/frameworks, so I guess it's here to stay.


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 120
> > 
> >
> > This suggests to me either that `Command` should be named `Invocation`, 
> > or that this variable should be named `command`. Given that `Command` is 
> > storing both the command and the arguments, it seems more fitting to name 
> > it `Invocation`.
> > 
> > Additionally, I think it would make a lot of sense to simply name it 
> > `Result` and move it into the `Subprocess` class. Similar to 
> > `Subprocess::IO`, we would have `Subprocess::Result` which captures the 
> > result of a subprocess call.
> > 
> > What do you think?

Done.
Renamed `Command` -> `Invocation`
Renamed `CommandResult` -> `Subprocess::Result`

Thanks for the suggestion!
(I was unhappy about naming too)


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 135-152
> > 
> >
> > Could you explain why these aren't symmetric? That is, why is `stdout_` 
> > not a `Option` or why is `stderr_` not a `std::string`?

It is safe to assume that every shell command will *always* emit to `stdout` 
(possibly an empty string) - while, not necessarily so to `stderr`.
It is probably a "distinction without a difference" but I believe it makes the 
caller's code easier to write and removes unnecessary (and tedious) `.isSome()` 
and `.get()` for stdout; while allowing for the possibility that there is no 
`stderr` available.

Obviously, we could make `stderr_` a `string` too - but making it an `Option` 
seemed to me to be more in line with Mesos' practices.


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 400
> > 
> >
> > Is there a reason why this is wrapped in a `std::shared_ptr`?

yes, because I need to pass it to the lambda and not doing so seemed to cause 
issues (if memory serves).
Wrapping it in a `shared_ptr` (instead of using a `raw` pointer) is also, in my 
understanding, "best practice" now in C++11?


> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 499-503
> > 
> >
> > Rather than exposing the `invokedWith_` member variable like this, can 
> > we introduce a `Subprocess` constructor that takes a `Command` instead? At 
> > which point we can do something like:
> > 
> > `Subprocess process(path == "sh" ? Command(argv[2]) : Command(path, 
> > argv));`

well, `invokedWith_` is already "exposed" to `subprocess` - as it's a 'friend' 
anyway.
But I have simplified the invocation, so that it reflects closely what you 
suggested.


- Marco


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


On Nov. 2, 2015, 7:22 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Nov. 2, 2015, 7:22 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
> ---
> 
> Jira: MESOS-3035
> 
> 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` (also introduced with this patch) 
> which
> contains useful information about the command invocation; the exit code;
> stdout and, optionally, stderr too.
> 
> Once the Future completes, if successful, the 

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

---
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.


Changes
---

Addressed all of mpark comments


Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035


Repository: mesos


Description (updated)
---

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 (updated)
---

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: Simplified the caller interface to process::Subprocess

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37336]

All tests passed.

- Mesos ReviewBot


On Nov. 2, 2015, 7:22 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Nov. 2, 2015, 7:22 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
> ---
> 
> Jira: MESOS-3035
> 
> 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` (also introduced with this patch) 
> which
> contains useful information about the command invocation; 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 
> 459825c188d56d25f6e2832e5a170d806e257d6b 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ac600a551fb1a7782ff33cce204b7819497ef54a 
> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Michael Park]

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



3rdparty/libprocess/include/process/subprocess.hpp (line 56)


Could you explain in what situations and why the list of arguments 
could/would be modified...?



3rdparty/libprocess/include/process/subprocess.hpp (lines 62 - 67)


`= default;` works fine here.

Just a future note: `for(std::string arg : other.args)` creates a copy for 
every string, which is then copied again on `args.push_back(arg)`.

Using `std::vector`'s copy constructor would've been better, `= default;` 
would be better.

You could even just omit it entirely if you wish, in which case it's 
equivalent to `= default;`



3rdparty/libprocess/include/process/subprocess.hpp (line 70)


`= {}` works for default construction.
`:` should be on the next line.



3rdparty/libprocess/include/process/subprocess.hpp (lines 73 - 75)


Please differentiate the name between `this->args` and `args`, and simply 
use `std::vector`'s copy constructor rather than manually looping.



3rdparty/libprocess/include/process/subprocess.hpp (line 100)


This should be just `f.get();` right?



3rdparty/libprocess/include/process/subprocess.hpp (line 113)


`:` on the next line.



3rdparty/libprocess/include/process/subprocess.hpp (line 120)


This suggests to me either that `Command` should be named `Invocation`, or 
that this variable should be named `command`. Given that `Command` is storing 
both the command and the arguments, it seems more fitting to name it 
`Invocation`.

Additionally, I think it would make a lot of sense to simply name it 
`Result` and move it into the `Subprocess` class. Similar to `Subprocess::IO`, 
we would have `Subprocess::Result` which captures the result of a subprocess 
call.

What do you think?



3rdparty/libprocess/include/process/subprocess.hpp (lines 135 - 152)


Could you explain why these aren't symmetric? That is, why is `stdout_` not 
a `Option` or why is `stderr_` not a `std::string`?



3rdparty/libprocess/include/process/subprocess.hpp (line 293)


`s/writing to the stream/writing to stderr/`

To keep consistency with the `stdout` version above.



3rdparty/libprocess/include/process/subprocess.hpp (line 400)


Is there a reason why this is wrapped in a `std::shared_ptr`?



3rdparty/libprocess/src/subprocess.cpp (line 219)


Can we not just call `cleanup()` here?



3rdparty/libprocess/src/subprocess.cpp (line 225)


Remove newline.



3rdparty/libprocess/src/subprocess.cpp (line 254)


Indented 1 level too deep.



3rdparty/libprocess/src/subprocess.cpp (lines 499 - 503)


Rather than exposing the `invokedWith_` member variable like this, can we 
introduce a `Subprocess` constructor that takes a `Command` instead? At which 
point we can do something like:

`Subprocess process(path == "sh" ? Command(argv[2]) : Command(path, argv));`


- Michael Park


On Nov. 2, 2015, 7:22 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Nov. 2, 2015, 7:22 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
> ---
> 
> Jira: MESOS-3035
> 
> 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` (also introduced with this patch) 
> which
> contains useful information about the command invocation; 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
> -
> 
>   

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

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

(Updated Nov. 2, 2015, 7:22 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
---

Jira: MESOS-3035

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` (also introduced with this patch) 
which
contains useful information about the command invocation; 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 
459825c188d56d25f6e2832e5a170d806e257d6b 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
ac600a551fb1a7782ff33cce204b7819497ef54a 

Diff: https://reviews.apache.org/r/37336/diff/


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

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

(Updated Oct. 18, 2015, 3:22 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Fixed compile error which was not caught by local `make && make check`


Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035


Repository: mesos


Description
---

Jira: MESOS-3035

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` (also introduced with this patch) 
which
contains useful information about the command invocation; 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 
d2341a53aac7c779668ee80983f73158fd44bff5 
  3rdparty/libprocess/src/subprocess.cpp 
a457cbe35ad33531c49f74550cd570cf28758f5d 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
ac600a551fb1a7782ff33cce204b7819497ef54a 

Diff: https://reviews.apache.org/r/37336/diff/


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37336]

All tests passed.

- Mesos ReviewBot


On Oct. 18, 2015, 3:22 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Oct. 18, 2015, 3:22 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
> ---
> 
> Jira: MESOS-3035
> 
> 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` (also introduced with this patch) 
> which
> contains useful information about the command invocation; 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 
> d2341a53aac7c779668ee80983f73158fd44bff5 
>   3rdparty/libprocess/src/subprocess.cpp 
> a457cbe35ad33531c49f74550cd570cf28758f5d 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ac600a551fb1a7782ff33cce204b7819497ef54a 
> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

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

(Updated Oct. 16, 2015, 11:03 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 (updated)
---

Jira: MESOS-3035

The original API for `process::Subprocess` still left a lot of legwork
to do for the caller; we have now added an `executed()` method
that returns a `Future` (also introduced with this patch) 
which
contains useful information about the command invocation; 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 
d2341a53aac7c779668ee80983f73158fd44bff5 
  3rdparty/libprocess/src/subprocess.cpp 
d6ea62ed1c914d34e0e189395831c86fff8aac22 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
ab7515325e5db0a4fd222bb982f51243d7b7e39d 

Diff: https://reviews.apache.org/r/37336/diff/


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

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

(Updated Oct. 16, 2015, 11:05 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Modified following conversation w/ benh/joris


Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035


Repository: mesos


Description (updated)
---

Jira: MESOS-3035

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` (also introduced with this patch) 
which
contains useful information about the command invocation; 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 
d2341a53aac7c779668ee80983f73158fd44bff5 
  3rdparty/libprocess/src/subprocess.cpp 
a457cbe35ad33531c49f74550cd570cf28758f5d 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
ac600a551fb1a7782ff33cce204b7819497ef54a 

Diff: https://reviews.apache.org/r/37336/diff/


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Joris Van Remoortere]

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



3rdparty/libprocess/include/process/subprocess.hpp (line 68)


`{` on new line



3rdparty/libprocess/include/process/subprocess.hpp (line 94)


full words for variable names.



3rdparty/libprocess/include/process/subprocess.hpp (line 281)


Is this API something you agreed on with your shepherd? I'm curious why we 
provide a blocking function rather than returning a future with the provided 
timeout.



3rdparty/libprocess/include/process/subprocess.hpp (line 292)


If this value is not meaningful until wait is finished, why not make this a 
future bound by that condition rather than providing undefined behavior?


- Joris Van Remoortere


On Oct. 12, 2015, 12:29 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Oct. 12, 2015, 12:29 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
> ---
> 
> Jira: MESOS-3035
> 
> The original API for `process::Subprocess` still left a lot of legwork
> to do for the caller; we have now added a `wait(Duration timeout)` method
> that returns a `CommandResult` (also introduced with this patch) which
> contains useful information about the command invocation; the exit code;
> stdout and, optionally, stderr too.
> 
> The `wait()` method will wait for both the process to terminate, and
> stdout/stderr to be available to read from; it also "unpacks" them into
> ready-to-use `string`s.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> d2341a53aac7c779668ee80983f73158fd44bff5 
>   3rdparty/libprocess/src/subprocess.cpp 
> d6ea62ed1c914d34e0e189395831c86fff8aac22 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ab7515325e5db0a4fd222bb982f51243d7b7e39d 
> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

> On Oct. 12, 2015, 4:08 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 281
> > 
> >
> > Is this API something you agreed on with your shepherd? I'm curious why 
> > we provide a blocking function rather than returning a future with the 
> > provided timeout.

uhm... `wait()` is by definition blocking :)
following our conversation, I'm going to change the name (and the signature) to 
return a Future instead


> On Oct. 12, 2015, 4:08 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 292
> > 
> >
> > If this value is not meaningful until wait is finished, why not make 
> > this a future bound by that condition rather than providing undefined 
> > behavior?

With the new signature, yes this will change too.
BTW this is not "undefined" - it is created in a way that it makes sense even 
in the case of failures.


- Marco


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


On Oct. 12, 2015, 12:29 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Oct. 12, 2015, 12:29 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
> ---
> 
> Jira: MESOS-3035
> 
> The original API for `process::Subprocess` still left a lot of legwork
> to do for the caller; we have now added a `wait(Duration timeout)` method
> that returns a `CommandResult` (also introduced with this patch) which
> contains useful information about the command invocation; the exit code;
> stdout and, optionally, stderr too.
> 
> The `wait()` method will wait for both the process to terminate, and
> stdout/stderr to be available to read from; it also "unpacks" them into
> ready-to-use `string`s.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> d2341a53aac7c779668ee80983f73158fd44bff5 
>   3rdparty/libprocess/src/subprocess.cpp 
> d6ea62ed1c914d34e0e189395831c86fff8aac22 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ab7515325e5db0a4fd222bb982f51243d7b7e39d 
> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 103-110
> > 
> >
> > Does it make sense to aggregate these into a `Result`? The 
> > `Some` case would be stdout, the `Error` case would be stederr, and the 
> > `None` case would represent not known yet?
> > If you don't need the `None` case then you can use a `Try`.

I like the idea and it would be a very elegant solution; unfortunately, I 
suspect it may not work in the more general case.

In my experience, Linux processes emit to both stdout and stderr (sometimes, 
even though there may be no error -- eg `rsync` and `tar` do, and, for 
non-fatal errors, they happily carry on; and can even be "forced" to continue 
in the presence of errors) - so, sometimes, you need both.

Also, bearing in mind there may be a bunch of output on `stdout` *before* the 
error, which then manifests itself in `stderr`: but one really needs to look at 
both to really do discovery.

Does it make sense?


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 350-354
> > 
> >
> > The indentation is a little different from your comment in 
> > `subprocess.hpp`. Let's be consistent between them, ideally also with the 
> > rest of the codebase if you can find some good examples.

so, this is `subprocess.hpp` and having looked at all of them, I notice that 
the *NOTE* pattern is not used; so I've changed it to `NOTE:`
However, they all seem aligned the same way (no indent).

I'll keep an eye for other places in my diff that may have different 
indentantion and will fix them too.


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 197
> > 
> >
> > I don't think it hurts to take timeout by const ref here. It helps the 
> > reader understand you don't intend to copy and modify it.
> > `const Duration& timeout`.
> > I acknowledge your have knowledge of the internal layout of the 
> > datastructure and know it's equally cheap to copy it. If someone came along 
> > in the future and added to the internal state of `Duration` then they 
> > wouldn't have to refactor your code :-)

You're being generous here :)
(but, really, can't think of a good reason why I didn't use a const & in the 
first place - it's my go-to choice...)
Fixed!


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 199
> > 
> >
> > I believe the issues you are running into regarding `FIXME(marco)` are 
> > rooted in your promise not having a future associated with it.
> > 
> > Usually the patter we follow is:
> > ```
> > // Create a promise.
> > Promise* promise = new promise();
> > 
> > // Get the future from the promise.
> > Future future = promise->future();
> > 
> > // Attach any mandatory chained functions to the future.
> > future.then([](){ ...; });
> > 
> > // Schedule our async action, and fulfill the promise inside that 
> > action.
> > io::read(fd).then([=]() {
> >   promise->set(...);
> > }).onFailed([=](const std::string& err) {
> >   promise->fail(err);
> > });
> > 
> > // Return the future to the user for them to attach any of their own 
> > callbacks.
> > return future.
> > ```
> > 
> > Feel free to sync with MPark or me to understand this further offline.

right - that's the pattern I had actually used in my preliminary tests; and it 
worked.
What throws a spanner in the works are a couple of things:

1) the Future returned from `await()` - not sure what to do with it:
```
  // We need to wait on the process to complete, as well as for
  // stdout and stderr to be available.
  Future, Future, Future>> waitRes =
  await(status(), stdout(), stderr());
```

2) returning a `Future` to the caller, instead of waiting inside the `wait()` 
method makes the caller API ugly:
```
Future future = subprocess.wait();
await(future, Seconds(5));
```
or something along those lines - I wanted to actually "compress" the two calls 
into one.

The question I had was more along the lines: given the current code (and the 
fact that it seems to "work as intended") would you be happy as to where it 
currently stands? or am I missing something that will come back and bite us?

Thanks!


- Marco


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


On Aug. 

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Joris Van Remoortere]

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


Hi Marco,

Sorry for the delay in looking at your review. I was able to dig in and 
hopefully identify the root cause of the issues you've left open with 
`FIXME(marco)`. Feel free to check with MPark or me offline for more 
information.
I left some preliminary explanatation at line 199 in `subprocess.cpp`.

I also pointed out some quick style things I noticed along the way. I figured I 
would share these early as it will give you the opportunity to clean up the 
review some more and reduce the number of iterations at the end :-)


3rdparty/libprocess/include/process/subprocess.hpp (lines 103 - 110)


Does it make sense to aggregate these into a `Result`? The 
`Some` case would be stdout, the `Error` case would be stederr, and the `None` 
case would represent not known yet?
If you don't need the `None` case then you can use a `Try`.



3rdparty/libprocess/include/process/subprocess.hpp (line 254)


According to the doxygen style guide we also end `@return` comments with a 
period. Here and elsehwere in this patch.



3rdparty/libprocess/include/process/subprocess.hpp (lines 350 - 354)


The indentation is a little different from your comment in 
`subprocess.hpp`. Let's be consistent between them, ideally also with the rest 
of the codebase if you can find some good examples.



3rdparty/libprocess/src/subprocess.cpp (lines 185 - 186)


Here are 2 options:
```
return out().isSome() ? 
  io::read(out().get()) : 
  Failure("Cannot obtain stdout for PID: " + stringify(data->pid));
```

```
if (out().isNone()) {
  return Failure(...);
}

return io::read(out().get());
```



3rdparty/libprocess/src/subprocess.cpp (line 197)


I don't think it hurts to take timeout by const ref here. It helps the 
reader understand you don't intend to copy and modify it.
`const Duration& timeout`.
I acknowledge your have knowledge of the internal layout of the 
datastructure and know it's equally cheap to copy it. If someone came along in 
the future and added to the internal state of `Duration` then they wouldn't 
have to refactor your code :-)



3rdparty/libprocess/src/subprocess.cpp (line 199)


I believe the issues you are running into regarding `FIXME(marco)` are 
rooted in your promise not having a future associated with it.

Usually the patter we follow is:
```
// Create a promise.
Promise* promise = new promise();

// Get the future from the promise.
Future future = promise->future();

// Attach any mandatory chained functions to the future.
future.then([](){ ...; });

// Schedule our async action, and fulfill the promise inside that action.
io::read(fd).then([=]() {
  promise->set(...);
}).onFailed([=](const std::string& err) {
  promise->fail(err);
});

// Return the future to the user for them to attach any of their own 
callbacks.
return future.
```

Feel free to sync with MPark or me to understand this further offline.



3rdparty/libprocess/src/subprocess.cpp (line 207)


You can indent by 2 spaces for an expression continuation.



3rdparty/libprocess/src/subprocess.cpp (lines 207 - 209)


I think your lambda indentation is off
```
[this](const tuple, 
   Future, 
   Future>& futuresTuple)
```



3rdparty/libprocess/src/subprocess.cpp (lines 232 - 233)


indent by 2, not 4. Elsewhere as well.



3rdparty/libprocess/src/subprocess.cpp (lines 239 - 240)


I would leave a space before the return.


- Joris Van Remoortere


On Aug. 20, 2015, 8:03 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> ---
> 
> (Updated Aug. 20, 2015, 8:03 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-3035
> 
> The original API for `process::Subprocess` still left a lot of legwork
> to 

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37336]

All tests passed.

- Mesos ReviewBot


On Aug. 20, 2015, 8:03 a.m., Marco Massenzio wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37336/
 ---
 
 (Updated Aug. 20, 2015, 8:03 a.m.)
 
 
 Review request for mesos and Joris Van Remoortere.
 
 
 Bugs: MESOS-3035
 https://issues.apache.org/jira/browse/MESOS-3035
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Jira: MESOS-3035
 
 The original API for `process::Subprocess` still left a lot of legwork
 to do for the caller; we have now added a `wait(Duration timeout)` method
 that returns a `CommandResult` (also introduced with this patch) which
 contains useful information about the command invocation; the exit code;
 stdout and, optionally, stderr too.
 
 The `wait()` method will wait for both the process to terminate, and
 stdout/stderr to be available to read from; it also unpacks them into
 ready-to-use `string`s.
 
 
 Diffs
 -
 
   3rdparty/libprocess/include/process/subprocess.hpp 
 d2341a53aac7c779668ee80983f73158fd44bff5 
   3rdparty/libprocess/src/subprocess.cpp 
 d6ea62ed1c914d34e0e189395831c86fff8aac22 
   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
 ab7515325e5db0a4fd222bb982f51243d7b7e39d 
 
 Diff: https://reviews.apache.org/r/37336/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Marco Massenzio