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

Ship it!



src/examples/test_hook_module.cpp (line 166)
<https://reviews.apache.org/r/36580/#comment146668>

    I'd explicitly call out the test, so that a reader of the code can 
understand the global invariant here.



src/hook/manager.cpp (line 212)
<https://reviews.apache.org/r/36580/#comment146665>

    Why do we copy the result into a TaskStatus only to return it below? seems 
like the code here should just be:
    
    if (result.isSome()) {
      return result.get();
    } else if (result.isError()) {
      LOG(WARNING) << "Slave TaskStatus label decorator hook failed for "
                   << "module '" << name << "': " << result.error();
    }
    
    return status.labels();
    
    And then we can take in TaskStatus as a const&. I see that this pattern is 
used everywhere so let's not do this now but unless I'm missing something we 
should circle back.



src/tests/hook_tests.cpp (line 509)
<https://reviews.apache.org/r/36580/#comment146666>

    Here and everywhere else in this review and other reviews, the expected 
value comes first, and the actual value comes second. Again, since all this 
code looks like this we can take care of it in a subsequent clean up review.


- Benjamin Hindman


On July 21, 2015, 4:46 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36580/
> -----------------------------------------------------------
> 
> (Updated July 21, 2015, 4:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This allows Slave modules to expose some information to the
> frameworks as well as Mesos-DNS via state.json.
> 
> 
> Diffs
> -----
> 
>   include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f 
>   src/examples/test_hook_module.cpp d61cd557d8e44e5089f324edf97b0335a4ededab 
>   src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc 
>   src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf 
>   src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 
>   src/tests/hook_tests.cpp 09205fb89925c22b1157294a756db87d911a63db 
> 
> Diff: https://reviews.apache.org/r/36580/diff/
> 
> 
> Testing
> -------
> 
> make check with a new hook test.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Reply via email to