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 Tue, 23 May 2017 07:31:37 GMT


> On May 12, 2017, 10:13 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp
> > Lines 157-158 (original), 155-156 (patched)
> > <https://reviews.apache.org/r/55895/diff/6/?file=1713122#file1713122line157>
> >
> >     Sorry I overlooked this in my last around of review but I had this question
on https://reviews.apache.org/r/55897/diff/2/?file=1667426#file1667426line185:
> >     
> >     ```
> >     I have questions about the need for soft limit below but I don't recall the
reason for setting the soft limits earlier "just for consistency". Soft limits is one of the
low-level system's funtionality that we didn't use. If we didn't use it, I am not sure about
the need for our util or the reader to be aware of it (the concept of soft limit)?
> >     ```
> >     
> >     Can we get rid of the soft limits?
> 
> James Peach wrote:
>     We set the soft limit because otherwise it would look weird to have a hard limit
but no soft limit. It's easy to set and I don't see any upside in leaving it unset, do you?
> 
> Jiang Yan Xu wrote:
>     How is it weird? These are two different concepts and we are not using soft limits.
IIUC to soft limits depend on hard limits but not vice versa.
>     
>     I as a reader needed to read additional docs to understand what it is and as a reviewer
had to reason about whether the code is using soft limits correctly. The behavior of soft
limits depend on other varibles which are not explicitly set or documented here and seemingly
it's only because we are setting the two with the same value that the soft limits don't kick
in.
>     
>     Code with no effect means redundancy but my point is more this is an additional concept
that we don't need to understand. There are other fields and concepts in http://xfs.org/docs/xfsdocs-xml-dev/XFS_Filesystem_Structure/tmp/en-US/html/Internal_Inodes.html
and other features provieded by XFS that I'd be happy to be ignorant of. :)

Chatted offline. IMO the soft limits is a feature that we didn't use and thus could be omitted
but jpeach felt it's an integral part of the XFS quota system and the reader should understand.


Let's keep the code that sets the soft limit but I feel the following comment would help explain
better:

a/Functionally all we need is the hard quota./Functionally all we need is the hard limit;
the soft limit has no effect when it is the same as the hard limit.

I could take care of if no objection.


- Jiang Yan


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


On May 16, 2017, 4:09 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55895/
> -----------------------------------------------------------
> 
> (Updated May 16, 2017, 4:09 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/7/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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