mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 55895: Extract a BasicBlocks class for disk block arithmetic.
Date Sat, 29 Apr 2017 05:41:56 GMT

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


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