----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9464/#review16744 -----------------------------------------------------------
src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35580> period. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35582> I'd kill this section, since CFS constants _are_ part of CPU subsystem contants, right? src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35583> This seems evident enough to me from the variable name, let's kill this comment. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35584> Can you add this to the end of the next line? // 100ms (Linux default). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35561> sorry for the misguidance, use CHECK_SOME instead: CHECK_SOME(exists) << "Failed to determine if cpu.cfs_quota_us exists"; CHECK_SOME will append the error message internally (see check_some.hpp) src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35562> I'm guessing the lack of a semi-colon would have prevented this from compiling ;) src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35564> We typically use the word "Failed" in conjunction with a failed operation. Here the operation succeded, and cpu.cfs_quota_us is indeed not present on the system. So rather than causing alarm to the log consumer (seeing a log line with "Failed"), I think it's more appropriate to print the fact that 'cpu.cfs_quota_us' is not present. Since we're now using cfsbc if present, WARNING is still the right thing (for now). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35571> s/.// We consistently don't use periods on the end of log messages. This is for several reasons: 1. Log messages with an expression at the end, are more clunky to append the period to: LOG(INFO) << "Updated 'cpu.shares' to " << shares << "."; 2. When printing variables, periods can be slightly ambiguous to the uninitiated: "Updated foo to ." Was foo updated to "."? Was foo updated to "", and the log line ended with a period? We use single quotes around values on many occasions to avoid ambiguity, but there are still many cases where we do not. 3. Some information looks a bit strange with a trailing period: "to 4.0." src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35585> The !exists.get() case is when cfs is unavailable, right? This case is a failed operation. I would return Try::error in this case instead (with a "Failed" message). If you want you can add the else case below to print a WARNING that cfs isn't being applied (when !exists.get()). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35575> We use 2 space indents on line continuation. Unless the line continues on a open paren. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9464/#comment35573> Thanks! - Ben Mahler On Feb. 18, 2013, 11:51 p.m., David Mackey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9464/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2013, 11:51 p.m.) > > > Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler. > > > Description > ------- > > Initial support for CPU "hard limits" via CFS Bandwidth Control in cgroups. > > CFS is unique relative to existing Mesos cgroups support in that it is a > "subfeature" of an already supported cgroups subsystem, cpu. Also, there are > two "tunables" for configuring CFS bandwidth limiting. > > There are 4 approaches one could take: > 1) Use the CFS bandwidth limiting if the feature is present > 2) Expose as separate flag, eg "cpu,cfs,memory,freezer" > 3) Add feature flag support to subsystems via an additional delimiter, eg > "cpu+cfs,memory,freezer". > 4) Add an additional control flag via some other means > > Option 2's downside breaks the 1:1 mapping between cgroups subsystems and a > cgroups resource flag. > Option 3's downside is it greatly increases complexity of parsing cgroups > subsystem flags. > > This diff takes option 1. > > > This addresses bug MESOS-315. > https://issues.apache.org/jira/browse/MESOS-315 > > > Diffs > ----- > > src/slave/cgroups_isolation_module.cpp 14f549e > > Diff: https://reviews.apache.org/r/9464/diff/ > > > Testing > ------- > > make check + additional testing > > > Thanks, > > David Mackey > >
