----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25922/#review54294 -----------------------------------------------------------
include/mesos/mesos.proto <https://reviews.apache.org/r/25922/#comment94311> I think we agreed to call these blk_read_total_iops, etc., to match the cgroup statistic and other naming? For cpus and memory we also include the limits that are in place, e.g., cpus_limit; add these for blk too. src/linux/cgroups.hpp <https://reviews.apache.org/r/25922/#comment94314> This seems generally useful, probably belongs in linux/fs.hpp src/linux/cgroups.hpp <https://reviews.apache.org/r/25922/#comment94324> s/aggregated stat information/aggregate statistics/ src/linux/cgroups.hpp <https://reviews.apache.org/r/25922/#comment94323> aggregateStatistics? src/linux/cgroups.hpp <https://reviews.apache.org/r/25922/#comment94315> no snake case, unless it's matching to a cgroup special file. put in linux/fs.hpp src/linux/cgroups.hpp <https://reviews.apache.org/r/25922/#comment94319> why a char * here: what about separate read_limit_in_bytes and write_limit_in_bytes? or an enum? src/linux/cgroups.hpp <https://reviews.apache.org/r/25922/#comment94320> s/bytes/iops/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94325> open brace on new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94326> new line before return src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94327> linux/fs.hpp src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94328> new line before return src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94329> open brace on new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94331> strcmp is pretty nasty... src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94330> new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94348> s/error/Error/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94332> new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94333> can avoid this if separate read write functions src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94338> s/part/partition/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94339> s/value/limit/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94334> new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94347> s/error/Error/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94335> new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94336> new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94344> s/error/Error/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94337> new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94340> s/part/partition/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94341> new line s/value/limit/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94345> s/error/Error/ src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94342> new line src/linux/cgroups.cpp <https://reviews.apache.org/r/25922/#comment94343> s/ / / src/slave/containerizer/isolators/cgroups/blkio.hpp <https://reviews.apache.org/r/25922/#comment94352> not needed here? src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94353> not needed? src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94354> new line src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94355> single line or correct indentation src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94358> This will cause container launch to fail. what about logging that this is not implemented and returning a Future<Limitation>() ? src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94359> s/r_bps/read_bps/ src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94360> ditto, and others src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94362> fix indentation, here and others, see style guide, e.g. Try<Nothing> write = cgroups::blkio::limit_in_bytes( hierarchy, info->cgroup, "read", r_bps.get()); src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94363> new line, here and others. src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94364> pull '<< w_iops.get()' up to match others if possible. src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94366> indentation src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94367> indentation src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/25922/#comment94368> new line src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/25922/#comment94370> alphabetize here too src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94400> Please comment on the need for memalign etc. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94399> Can you comment on why you're using pwrite/pread please. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94387> isn't io_size already a size_t? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94386> new line src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94394> Where do these values come from? Can you please write a brief comment on how they were chosen. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94373> no snake case, please src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94383> s/waited_for_write/writeWait/? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94376> push these down to where you use them? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94391> new line src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94385> done_writes could be replaced by waited_for_write > 0 ? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94382> pull closing bracket up with opening brace? alignment if (!done_writes && statistics.disk_write_total_bytes() >= bytes_threshold.bytes() && statistics.disk_write_total_iops() >= iops_threshold) { done_write = true; ... src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94378> new line src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94393> Perhaps comment that the min_wait for read/write is >> 1 second? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25922/#comment94395> Ditto, comments please. - Ian Downes On Sept. 22, 2014, 7:50 p.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25922/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2014, 7:50 p.m.) > > > Review request for mesos, Adam B and Benjamin Hindman. > > > Repository: mesos-git > > > Description > ------- > > Following from: r25105 > Currently there is no disk isolation in place and this affects an executor to > be starved of disk when another disk heavy operation such as copying a multi > gigabyte file is being performed by another executor. > > > Diffs > ----- > > include/mesos/mesos.proto be45494 > include/mesos/resources.hpp 0e37170 > src/Makefile.am 9b973e5 > src/common/resources.cpp e9a0c85 > src/linux/cgroups.hpp abf31df > src/linux/cgroups.cpp 5093b4c > 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 c38f876 > > Diff: https://reviews.apache.org/r/25922/diff/ > > > Testing > ------- > > make check > sudo bin/mesos-tests.sh --gtest_filter="*BlkIO*" > support/mesos-style.py > > > Thanks, > > Joris Van Remoortere > >
