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


One general comment: Should it be ResourceUsage::Executor or should we be a bit 
more specific, for example ResourceUsage::ExecutorUsage? You had one name 
conflict below where you had to call it 'entry'.
I am fine with either.


include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139631>

    s/allcoated/allocated/



include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139632>

    s/missing/absent/
    s/isolation module/containerizer/



src/slave/monitor.hpp
<https://reviews.apache.org/r/35260/#comment139637>

    Want to add a deprecation todo?



src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139634>

    Can you kill this?



src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139633>

    Can you kill this too?



src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139635>

    2 additional space indent as it is a argument wrap



src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139636>

    Do you know if it makes sense to const lambda::function<>?



src/slave/slave.hpp
<https://reviews.apache.org/r/35260/#comment139638>

    Should this also be virtual (taken it is public, which I assume is for 
testing purposes?)
    
    Maybe not needed yet, so no biggie.



src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139640>

    Mind adding a comment about the constraint?
    
    I can see you check A == B == C, but was a bit implicit.



src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139641>

    future->isReady() will always be false here. Is this what you want?



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35260/#comment139646>

    This doesn't test the same thing anymore? Shouldn't this be moved to a 
slave test instead of getting transformed into another test?


- Niklas Nielsen


On June 9, 2015, 11:54 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
> 
> (Updated June 9, 2015, 11:54 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2818
>     https://issues.apache.org/jira/browse/MESOS-2818
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored the ResourceMonitor to get statistics from the Slave.
> 
> 1) Modified ResourceUsage to include allocation information (see ticket for 
> reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get 
> statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87 
>   include/mesos/slave/resource_estimator.hpp 
> 7f78fd86760397d5b885a2c00b41081d0b546cdd 
>   src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7 
>   src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c 
>   src/slave/resource_estimators/fixed.cpp 
> 3efa18d7e3c6ac62e67f75ea45a832f3f6349036 
>   src/slave/resource_estimators/noop.hpp 
> 7a44e6deefc9c1985c14d076a427aa5c654aa1bb 
>   src/slave/resource_estimators/noop.cpp 
> 5f135ff37e84c248ede29bbe4a7d1b8319417e20 
>   src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e 
>   src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac 
>   src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 
>   src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a 
>   src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0 
> 
> Diff: https://reviews.apache.org/r/35260/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Reply via email to