mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joris Van Remoortere" <joris.van.remoort...@gmail.com>
Subject Re: Review Request 34309: Support manipulating scheduler policy on Linux.
Date Sat, 16 May 2015 19:31:46 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message