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


Great work Ian!

Could you add the dependent reviews for this chain? I'm missing some dependent 
code when trying to apply 34309 + 34310 on master right now.

Should we put in any guards in for the POSIX / LINUX mismatch bug mentioned 
here: http://linux.die.net/man/2/sched_getscheduler
If we don't need any guards, should we mention it at the call site or in a 
top-level comment in the header?

In previous projects I have used RAII to wrap these kinds of utilities. The 
advantage of that is that we always "clean up" back to the original policy upon 
destruction, rather than relying on manual calls to reset to the original 
policy. This is a larger pattern change for more than just this utility, but I 
thought I would start the conversation. Maybe we can discuss it further at one 
of the community meetings?


src/linux/sched.hpp
<https://reviews.apache.org/r/34309/#comment135167>

    unused?



src/linux/sched.hpp
<https://reviews.apache.org/r/34309/#comment135162>

    In the comment below you use a `,` to split the `isSome` case from the 
`isNone` case. Let's be consistent? Maybe we can clarify the `caller` part as 
well:
    
    `Return the current scheduling policy for the specified pid, or for the 
caller if pid is None() or zero.`



src/linux/sched.hpp
<https://reviews.apache.org/r/34309/#comment135161>

    Should we add the pid for which this failed to the error message?



src/linux/sched.hpp
<https://reviews.apache.org/r/34309/#comment135164>

    Can we add some checks for the condition mentioned in the comment? (i.e. 
priority only making sense for the FIFO and RR policy). The EINVAL error code 
from sched_setscheduler() is rather over-loaded, so I think this is a great 
opportunity to help our users and give them a more helpful error message! :-)



src/linux/sched.hpp
<https://reviews.apache.org/r/34309/#comment135163>

    Should we add the pid for which this failed to the error message?



src/linux/sched.hpp
<https://reviews.apache.org/r/34309/#comment135160>

    missing trailing `{` for namespace.



src/tests/sched_tests.cpp
<https://reviews.apache.org/r/34309/#comment135168>

    Do you want to add a TODO for some tests that:
    1) Actually test that the priority / interruption behavior is as expected
    2) Shows people how to use this / explains how it works
    
    I think 1) and 2) can be done in the same test with nice comments :-)



src/tests/sched_tests.cpp
<https://reviews.apache.org/r/34309/#comment135166>

    1) This test requires elevated (CAP_SYS) permissions. We can use the 
`ROOT_XXX` prefix to signify this so it doesn't fail during non-priveleged user 
test runs? I don't know if we want to introduce more filters for specific 
permission requirements (like `CAPSYS_XXX`? That's probably a wishlist thing :-)
    
    2) You have a trailing `;` here. This doesn't compile for me.



src/tests/sched_tests.cpp
<https://reviews.apache.org/r/34309/#comment135165>

    Could you elaborate on this comment and explain what you are testing in the 
child? I don't think it's clear.
    
    Should we also test the inheritance? (i.e. fork while in the IDLE policy, 
and then verify in the child?)


- Joris Van Remoortere


On May 16, 2015, 1:14 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34309/
> -----------------------------------------------------------
> 
> (Updated May 16, 2015, 1:14 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-2652
>     https://issues.apache.org/jira/browse/MESOS-2652
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Support manipulating scheduler policy on Linux.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/linux/sched.hpp PRE-CREATION 
>   src/tests/sched_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34309/diff/
> 
> 
> Testing
> -------
> 
> Added test.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Reply via email to