mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joris Van Remoortere" <joris.van.remoort...@gmail.com>
Subject Re: Review Request 37336: Simplified the caller interface to process::Subprocess
Date Mon, 07 Sep 2015 23:14:48 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message