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



src/slave/monitor.hpp
<https://reviews.apache.org/r/33876/#comment134204>

    No need to use the prefix `ResourcMonitor::` for nested type.



src/slave/monitor.cpp
<https://reviews.apache.org/r/33876/#comment134209>

    Please move this continuation after `usages()` below.



src/slave/monitor.cpp
<https://reviews.apache.org/r/33876/#comment134231>

    Hum, this looks like a bug to me? Since you capture `containerId` by 
reference, what if by the time `onFailed` is called, the `containerId` object 
is no longer valid (e.g., `monitored` has been changed because of a new 
container coming in)?
    
    Similar to that in
    
http://stackoverflow.com/questions/6775174/lambda-should-capturing-const-reference-by-reference-yield-undefined-behaviour
    
    The same for 'executorInfo'.
    
    You probably should consider do the following:
    ```
    Future<Usage> ResourceMonitorProcess::usage(ContainerID containerId)
    {
      ...
      ExecutorInfo executorInfo = monitored[containerId];
      
      ...
      .onFailed([conatinerId, executorInfo](...) {})
      ...
    }
    ```



src/slave/monitor.cpp
<https://reviews.apache.org/r/33876/#comment134240>

    The prefix `ResourceMonitor::` is not needed here


- Jie Yu


On May 11, 2015, 6:20 p.m., Niklas Nielsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33876/
> -----------------------------------------------------------
> 
> (Updated May 11, 2015, 6:20 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added usages() to resource monitor to enable internal components tapping into 
> resource statistics.
> 
> 
> Diffs
> -----
> 
>   src/slave/monitor.hpp 69c60a10187f8ea617c6be9738b28e8103e0ed27 
>   src/slave/monitor.cpp 398af010564e999b46e38560ba1e652261a9420c 
> 
> Diff: https://reviews.apache.org/r/33876/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Reply via email to