-----------------------------------------------------------
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)
<https://reviews.apache.org/r/43305/#comment179723>

    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)
<https://reviews.apache.org/r/43305/#comment179724>

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



src/linux/systemd.hpp (line 52)
<https://reviews.apache.org/r/43305/#comment179842>

    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)
<https://reviews.apache.org/r/43305/#comment179725>

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

Reply via email to