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/08/20 10:03:48 UTC
Re: Review Request 37336: Simplified the caller interface to
process::Subprocess
-----------------------------------------------------------
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.
Changes
-------
This now works and passes a sensible set of tests; still, there is some 'surprising' behavior and I'd like to understand that before committing.
Summary (updated)
-----------------
Simplified the caller interface to process::Subprocess
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 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 (updated)
-----
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
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 37336: Simplified the caller interface to
process::Subprocess
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 103-110
> > <https://reviews.apache.org/r/37336/diff/6/?file=1044858#file1044858line103>
> >
> > Does it make sense to aggregate these into a `Result<std::string>`? 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<std::string>`.
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
> > <https://reviews.apache.org/r/37336/diff/6/?file=1044858#file1044858line350>
> >
> > 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
> > <https://reviews.apache.org/r/37336/diff/6/?file=1044859#file1044859line197>
> >
> > 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
> > <https://reviews.apache.org/r/37336/diff/6/?file=1044859#file1044859line199>
> >
> > 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<Nothing>* promise = new promise();
> >
> > // Get the future from the promise.
> > Future<Nothing> 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<tuple<Future<Option<int>>, Future<string>, Future<string>>> 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<CommandResult> 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. 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
>
>
Re: Review Request 37336: Simplified the caller interface to
process::Subprocess
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/37336/#comment154076>
Does it make sense to aggregate these into a `Result<std::string>`? 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<std::string>`.
3rdparty/libprocess/include/process/subprocess.hpp (line 254)
<https://reviews.apache.org/r/37336/#comment154077>
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)
<https://reviews.apache.org/r/37336/#comment154078>
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)
<https://reviews.apache.org/r/37336/#comment154079>
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)
<https://reviews.apache.org/r/37336/#comment154080>
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)
<https://reviews.apache.org/r/37336/#comment154085>
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<Nothing>* promise = new promise();
// Get the future from the promise.
Future<Nothing> 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)
<https://reviews.apache.org/r/37336/#comment154081>
You can indent by 2 spaces for an expression continuation.
3rdparty/libprocess/src/subprocess.cpp (lines 207 - 209)
<https://reviews.apache.org/r/37336/#comment154082>
I think your lambda indentation is off
```
[this](const tuple<Future<Option<int>>,
Future<string>,
Future<string>>& futuresTuple)
```
3rdparty/libprocess/src/subprocess.cpp (lines 232 - 233)
<https://reviews.apache.org/r/37336/#comment154083>
indent by 2, not 4. Elsewhere as well.
3rdparty/libprocess/src/subprocess.cpp (lines 239 - 240)
<https://reviews.apache.org/r/37336/#comment154084>
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 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
>
>