Re: Review Request 33876: Added usages() to resource monitor
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:
```
FutureUsage 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
Re: Review Request 33876: Added usages() to resource monitor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33876/#review83645 --- Ship it! Ship It! - Jie Yu On May 12, 2015, 8: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, 8: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
Re: Review Request 33876: Added usages() to resource monitor
--- 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. Changes --- Adressed Jie's comments. Repository: mesos Description --- Added usages() to resource monitor to enable internal components tapping into resource statistics. Diffs (updated) - src/slave/monitor.hpp 69c60a10187f8ea617c6be9738b28e8103e0ed27 src/slave/monitor.cpp 398af010564e999b46e38560ba1e652261a9420c Diff: https://reviews.apache.org/r/33876/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 33876: Added usages() to resource monitor
---
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:
```
FutureUsage 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
Re: Review Request 33876: Added usages() to resource monitor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33876/#review82627 --- Patch looks great! Reviews applied: [33875, 33876] All tests passed. - Mesos ReviewBot On May 6, 2015, 2:16 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33876/ --- (Updated May 6, 2015, 2:16 a.m.) Review request for mesos 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
