mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ian Downes" <ian.dow...@gmail.com>
Subject Re: Review Request 27111: Block IO Isolator: usage metrics
Date Sat, 25 Oct 2014 01:02:36 GMT

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


Close to ready. Major points are cleaning up the cgroups statistics functions and making the
test child safe.


src/linux/cgroups.hpp
<https://reviews.apache.org/r/27111/#comment99413>

    s/The a pair/The pair/



src/linux/cgroups.cpp
<https://reviews.apache.org/r/27111/#comment99414>

    We accept ">>" now.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/27111/#comment99481>

    I think it's error-prone to have the implicit <read, write> pairing and specifying
the file.
    
    What about hiding this function in an internal namespace and exposing the various statistics
explicitly under a descriptive namespace, e.g., perhaps something like this:
    
    Bytes blkio::statistics::bytes::{read,write}()
    
    uint64_t blkio::statistics::iops::{read,write}()
    
    Yes, converting to Bytes to return is unnecessary in your usage but it avoids any ambiguity
for the user of this library function.



src/linux/fs.cpp
<https://reviews.apache.org/r/27111/#comment99419>

    Any tests for this?



src/linux/fs.cpp
<https://reviews.apache.org/r/27111/#comment99417>

    move this to after the file.fail() check?



src/slave/containerizer/isolators/cgroups/blkio.cpp
<https://reviews.apache.org/r/27111/#comment99473>

    kill newline



src/slave/containerizer/isolators/cgroups/blkio.cpp
<https://reviews.apache.org/r/27111/#comment99482>

    What's the difference between blkio.throttle.io_service_bytes and  blkio.io_service_bytes?



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/27111/#comment99490>

    s/alligned/aligned/



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/27111/#comment99492>

    posix_memalign is definitely not async-signal-safe



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/27111/#comment99488>

    s/ret/write/? or count or ...
    something other than 'ret', please :-)
    
    pwrite() is not listed as async-signal-safe: http://man7.org/linux/man-pages/man7/signal.7.html



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/27111/#comment99489>

    ditto



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/27111/#comment99484>

    This "setup" function must be async-signal-safe as it's called between the fork and exec,
and we're forking from a threaded parent.
    
    http://man7.org/linux/man-pages/man7/signal.7.html
    
    Can you verify that everything in there is safe (and comment that it is) or put this code
into a separate helper binary?


- Ian Downes


On Oct. 23, 2014, 2:58 p.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27111/
> -----------------------------------------------------------
> 
> (Updated Oct. 23, 2014, 2:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ian Downes.
> 
> 
> Bugs: MESOS-1977
>     https://issues.apache.org/jira/browse/MESOS-1977
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Implement a block io isolator that just publishes read / write, bytes / operations per
second.
> A split of r25922 as suggested by Ian.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 6b93e90 
>   src/Makefile.am 2617f77 
>   src/linux/cgroups.hpp abf31df 
>   src/linux/cgroups.cpp 62df4b7 
>   src/linux/fs.hpp ac8b5f4 
>   src/linux/fs.cpp b01d14c 
>   src/slave/containerizer/isolators/cgroups/blkio.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/blkio.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/containerizer.cpp 9d08329 
>   src/tests/isolator_tests.cpp 52b38a3 
> 
> Diff: https://reviews.apache.org/r/27111/diff/
> 
> 
> Testing
> -------
> 
> make check
> sudo ./mesos-tests --gtest_filter="BlkIOIsolatorTest*"
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>


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