Re: Review Request 33876: Added usages() to resource monitor

[Niklas Nielsen]

> On May 11, 2015, 3:49 p.m., Jie Yu wrote:
> > src/slave/monitor.cpp, line 126
> > 
> >
> > 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 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

[Ian Downes]

> On May 11, 2015, 3:49 p.m., Jie Yu wrote:
> > src/slave/monitor.cpp, line 126
> > 
> >
> > 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 ResourceMonitorProcess::usage(ContainerID containerId)
> > {
> >   ...
> >   ExecutorInfo executorInfo = monitored[containerId];
> >   
> >   ...
> >   .onFailed([conatinerId, executorInfo](...) {})
> >   ...
> > }
> > ```
> 
> Niklas Nielsen wrote:
> Good catch! Should be fixed in the latest patch
> 
> Thanks!

Committed code captures executorInfo by reference rather than by value as 
suggested here.


- Ian


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

[Jie Yu]

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [33875, 33876]

All tests passed.

- Mesos ReviewBot


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

[Niklas Nielsen]

> On May 11, 2015, 3:49 p.m., Jie Yu wrote:
> > src/slave/monitor.cpp, line 289
> > 
> >
> > 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
> > 
> >
> > 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 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
> 
>

Re: Review Request 33876: Added usages() to resource monitor

[Niklas Nielsen]

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

[Jie Yu]

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



src/slave/monitor.hpp


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



src/slave/monitor.cpp


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



src/slave/monitor.cpp


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 ResourceMonitorProcess::usage(ContainerID containerId)
{
  ...
  ExecutorInfo executorInfo = monitored[containerId];
  
  ...
  .onFailed([conatinerId, executorInfo](...) {})
  ...
}
```



src/slave/monitor.cpp


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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [33875, 33876]

All tests passed.

- Mesos ReviewBot


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

[Niklas Nielsen]

> On May 6, 2015, 11:51 a.m., Jie Yu wrote:
> > src/slave/monitor.cpp, lines 89-90
> > 
> >
> > We've already done `using std::list` above. No need to have the `std::` 
> > prefix.
> > 
> > ALso, no need for `process::` prefix as well.
> > 
> > Here and everywhere else.

Good catch! Thanks!


> On May 6, 2015, 11:51 a.m., Jie Yu wrote:
> > src/slave/monitor.cpp, lines 91-100
> > 
> >
> > We typically put continuations below their parent (i.e., `usages()`). 
> > See `statistics` and its continuations.

Reordered in latest patch


> On May 6, 2015, 11:51 a.m., Jie Yu wrote:
> > src/slave/monitor.cpp, line 148
> > 
> >
> > No need to pass in `executorInfo` since you can get it from 
> > `monitored`? Do you want to return a Failure (or add a CHECK) to make sure 
> > containerId is in `monitored`?

It would be an extra lookup, but have removed the parameter and moved the 
lookup into usage()


> On May 6, 2015, 11:51 a.m., Jie Yu wrote:
> > src/slave/monitor.cpp, lines 151-152
> > 
> >
> > Use lambda here:) To match the semantics of the previous version, do 
> > you also want to install onDiscarded callback here?

Looks much better in a lambda! Thanks
Added the onDiscarded callback too


> On May 6, 2015, 11:51 a.m., Jie Yu wrote:
> > src/slave/monitor.cpp, lines 153-158
> > 
> >
> > We usually put onXXX at the end of the chain:
> > ```
> > containerizer->usage(...)
> >   .then(...)
> >   .onFailed(...);
> > ```
> > 
> > Also, could you please move the continuation `_usage` below?

Done


- Niklas


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


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

[Niklas Nielsen]

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

(Updated May 11, 2015, 11:20 a.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 (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

[Niklas Nielsen]

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

(Updated May 11, 2015, 11:18 a.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 (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

[Jie Yu]

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



src/slave/monitor.cpp


We've already done `using std::list` above. No need to have the `std::` 
prefix.

ALso, no need for `process::` prefix as well.

Here and everywhere else.



src/slave/monitor.cpp


We typically put continuations below their parent (i.e., `usages()`). See 
`statistics` and its continuations.



src/slave/monitor.cpp


No need to pass in `executorInfo` since you can get it from `monitored`? Do 
you want to return a Failure (or add a CHECK) to make sure containerId is in 
`monitored`?



src/slave/monitor.cpp


Use lambda here:) To match the semantics of the previous version, do you 
also want to install onDiscarded callback here?



src/slave/monitor.cpp


We usually put onXXX at the end of the chain:
```
containerizer->usage(...)
  .then(...)
  .onFailed(...);
```

Also, could you please move the continuation `_usage` below?


- Jie Yu


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

[Mesos ReviewBot]

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

Review Request 33876: Added usages() to resource monitor

[Niklas Nielsen]

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

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