Re: Review Request 43305: Moved systemd executor slice initialization logic.

[Joris Van Remoortere]

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

(Updated Feb. 10, 2016, 4:50 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


Bugs: MESOS-4636
https://issues.apache.org/jira/browse/MESOS-4636


Repository: mesos


Description
---

Moved systemd executor slice initialization logic.


Diffs (updated)
-

  src/linux/systemd.hpp dc8605b323cafdc0dde00d36a2cc5cf2c241e51c 
  src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
  src/slave/containerizer/mesos/linux_launcher.hpp 
b061981b063a67d837fe8e291921ca9c44d3ac40 
  src/slave/containerizer/mesos/linux_launcher.cpp 
c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
  src/slave/main.cpp 22b833044dc3f900e3b5ea509e6e545148649f48 

Diff: https://reviews.apache.org/r/43305/diff/


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 43305: Moved systemd executor slice initialization logic.

[Joseph Wu]

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




src/slave/containerizer/mesos/linux_launcher.cpp 


Looks like this breaks the `CGROUPS_ROOT_*` tests.  I'll have a patch up 
shortly to address this:

https://reviews.apache.org/r/43432/


- Joseph Wu


On Feb. 10, 2016, 8:50 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43305/
> ---
> 
> (Updated Feb. 10, 2016, 8:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Bugs: MESOS-4636
> https://issues.apache.org/jira/browse/MESOS-4636
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved systemd executor slice initialization logic.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.hpp dc8605b323cafdc0dde00d36a2cc5cf2c241e51c 
>   src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> b061981b063a67d837fe8e291921ca9c44d3ac40 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
>   src/slave/main.cpp 22b833044dc3f900e3b5ea509e6e545148649f48 
> 
> Diff: https://reviews.apache.org/r/43305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 43305: Moved systemd executor slice initialization logic.

[Joris Van Remoortere]

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

(Updated Feb. 9, 2016, 7:13 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


Repository: mesos


Description (updated)
---

Moved systemd executor slice initialization logic.


Diffs (updated)
-

  src/linux/systemd.hpp dc8605b323cafdc0dde00d36a2cc5cf2c241e51c 
  src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
  src/slave/containerizer/mesos/linux_launcher.hpp 
b061981b063a67d837fe8e291921ca9c44d3ac40 
  src/slave/containerizer/mesos/linux_launcher.cpp 
c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
  src/slave/main.cpp 22b833044dc3f900e3b5ea509e6e545148649f48 

Diff: https://reviews.apache.org/r/43305/diff/


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 43305: Moved systemd executor slice initialization logic.

[Benjamin Hindman]

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


Fix it, then Ship it!





src/linux/systemd.hpp (line 30)


As long as it's in the `systemd` namespace, can we drop the `SYSTEMD_` 
prefix?

And it's a bit wierd to call it "executors". I remember when this came up 
in the past and we were afraid that there might be other things that we want to 
run in there as well ... can we change this now easily?

Finally, can you add a comment that explains in more detail what all might 
be moved into this slice? Even if we can change the name away from "executors" 
a comment here would still be great.



src/linux/systemd.hpp (line 49)


Do you want to be explicit about moving it into the slice we've called out 
above?



src/linux/systemd.hpp (line 52)


Just a thought here. The abstraction we want here feels like it's a "move 
to a particular systemd slice". The fact that moving it to that slice "extends 
the lifetime" is Mesos specific, and in that way it seems a bit wierd to call 
this function 'extendLifetime' or at least have it in the general `systemd` 
namespace.



src/linux/systemd.cpp (line 79)


Why this `CHECK`? Why not return an error if you try and initialize systemd 
but it doesn't exist? This seems like a non-local assertion, i.e., the systemd 
"library" doesn't know all of its callers so why not just return an `Error`? I 
acknowledge that technically this is all Mesos, but the `CHECK` seems overly 
aggressive, unless I'm missing something?


- Benjamin Hindman


On Feb. 9, 2016, 7:13 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43305/
> ---
> 
> (Updated Feb. 9, 2016, 7:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved systemd executor slice initialization logic.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.hpp dc8605b323cafdc0dde00d36a2cc5cf2c241e51c 
>   src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> b061981b063a67d837fe8e291921ca9c44d3ac40 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
>   src/slave/main.cpp 22b833044dc3f900e3b5ea509e6e545148649f48 
> 
> Diff: https://reviews.apache.org/r/43305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 43305: Moved systemd executor slice initialization logic.

[haosdent huang]

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




src/linux/systemd.cpp (line 150)


indent is incorrect here?


- haosdent huang


On Feb. 7, 2016, 1:57 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43305/
> ---
> 
> (Updated Feb. 7, 2016, 1:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.hpp dc8605b323cafdc0dde00d36a2cc5cf2c241e51c 
>   src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> b061981b063a67d837fe8e291921ca9c44d3ac40 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
>   src/slave/main.cpp 22b833044dc3f900e3b5ea509e6e545148649f48 
> 
> Diff: https://reviews.apache.org/r/43305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 43305: Moved systemd executor slice initialization logic.

[Joris Van Remoortere]

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

Review request for mesos, Benjamin Hindman and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/linux/systemd.hpp dc8605b323cafdc0dde00d36a2cc5cf2c241e51c 
  src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
  src/slave/containerizer/mesos/linux_launcher.hpp 
b061981b063a67d837fe8e291921ca9c44d3ac40 
  src/slave/containerizer/mesos/linux_launcher.cpp 
c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
  src/slave/main.cpp 22b833044dc3f900e3b5ea509e6e545148649f48 

Diff: https://reviews.apache.org/r/43305/diff/


Testing
---


Thanks,

Joris Van Remoortere