----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55895/#review173411 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/xfs/utils.hpp Lines 46 (patched) <https://reviews.apache.org/r/55895/#comment246414> s/b/bytes/ src/slave/containerizer/mesos/isolators/xfs/utils.hpp Lines 49 (patched) <https://reviews.apache.org/r/55895/#comment246415> s/c/count/ or I wonder if s/c/blocks/ makes more sense? Not `count` isn't ok but you named the method `uint64_t blocks() const` and not `uint64_t count() const` after all? So for consistency stick to one? src/slave/containerizer/mesos/isolators/xfs/utils.hpp Lines 52 (patched) <https://reviews.apache.org/r/55895/#comment246416> A blank line above looks better with your current method grouping. src/slave/containerizer/mesos/isolators/xfs/utils.hpp Lines 58 (patched) <https://reviews.apache.org/r/55895/#comment246417> Styling issues: - Space after `;`. - s/block_size/blockSize/ (We don't use snake casing in Mesos unless it's a constant (then it's capitalized). (we do use snake casing in libprocess and stout, sigh, don't ask me why) However why a method? It's semantically a constant value so it's more idiomatic in Mesos to use a variable. Just move `static constexpr Bytes BASIC_BLOCK_SIZE = Bytes(512u);` into the class? src/slave/containerizer/mesos/isolators/xfs/utils.cpp Lines 252-254 (original), 248-250 (patched) <https://reviews.apache.org/r/55895/#comment246419> This is not intuitive to me. The assignments above `quota.d_blk_hardlimit = BasicBlocks(limit).blocks();` is very clear that `d_blk_hardlimit` is the number of blocks. Here `info.limit = BasicBlocks(quota.d_blk_hardlimit);` looks like `info.limit`'s unit is blocks but it's not due to the implicit conversion. How about we remove the implicit conversion to `Bytes` and just use the method `Bytes bytes() const`? src/tests/containerizer/xfs_quota_tests.cpp Lines 944 (patched) <https://reviews.apache.org/r/55895/#comment246423> Unless there's useful variables printed out, we typically capture these as comments in tests. (See the rest of this file) Comments give you a bit more flexibility in the placement and structure while attaching to stdout allows you to see it in the log. Consistency is more important though so let's not start a new style here yet. Let's chat if you feel strongly about advocating for this style. src/tests/containerizer/xfs_quota_tests.cpp Lines 948 (patched) <https://reviews.apache.org/r/55895/#comment246422> Be consistent in the use of unsigned literal. - Jiang Yan Xu On April 25, 2017, 3:37 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55895/ > ----------------------------------------------------------- > > (Updated April 25, 2017, 3:37 p.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-5116 > https://issues.apache.org/jira/browse/MESOS-5116 > > > Repository: mesos > > > Description > ------- > > Extract a BasicBlocks class to handle disk blocks to clarify block-based > arithmetic in the XFS disk isolator. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/xfs/utils.hpp > eddd4c37fb42339ca21ecb392dea47acf6b277bb > src/slave/containerizer/mesos/isolators/xfs/utils.cpp > 8018ad348d26bd962357543a5fb9f6cb43ff13b1 > src/tests/containerizer/xfs_quota_tests.cpp > 7beb60b059910a0d4451b1ace895a35dc974a043 > > > Diff: https://reviews.apache.org/r/55895/diff/4/ > > > Testing > ------- > > sudo make check (Fedora 25) > > > Thanks, > > James Peach > >
