This is an automatically generated e-mail. To reply, visit:

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

    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<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([=]() {
    }).onFailed([=](const std::string& err) {
    // Return the future to the user for them to attach any of their own 
    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<Option<int>>, 
                       Future<string>>& 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 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

Reply via email to