> On May 11, 2015, 3:49 p.m., Jie Yu wrote:
> > src/slave/monitor.cpp, line 289
> > <https://reviews.apache.org/r/33876/diff/3/?file=955511#file955511line289>
> >
> >     The prefix `ResourceMonitor::` is not needed here

This one is needed, to compile


> On May 11, 2015, 3:49 p.m., Jie Yu wrote:
> > src/slave/monitor.cpp, line 126
> > <https://reviews.apache.org/r/33876/diff/3/?file=955511#file955511line126>
> >
> >     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](...) {})
> >       ...
> >     }
> >     ```

Good catch! Should be fixed in the latest patch

Thanks!


- Niklas


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


On May 12, 2015, 1:55 p.m., Niklas Nielsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33876/
> -----------------------------------------------------------
> 
> (Updated May 12, 2015, 1:55 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