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