> 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](...) {}) > > ... > > } > > ``` > > Niklas Nielsen wrote: > Good catch! Should be fixed in the latest patch > > Thanks! > > Ian Downes wrote: > Committed code captures executorInfo by reference rather than by value as > suggested here.
This patch changes to making copies instead: https://reviews.apache.org/r/34728/ Working on a test to exercise this. - 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 > >
