You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/01/24 23:59:36 UTC
Review Request 55895: Add zero byte disk resource support to XFS.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-5393
https://issues.apache.org/jira/browse/MESOS-5393
Repository: mesos
Description
-------
Support containers with zero byte disk resources in the XFS
isolator. This works by setting the quota to the minimum amount
(1 block), since the quota system defines 0 as unlimited.
Diffs
-----
src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
Diff: https://reviews.apache.org/r/55895/diff/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/#review175768
-----------------------------------------------------------
Ship it!
Ship It!
- Jiang Yan Xu
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
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/
-----------------------------------------------------------
(Updated May 16, 2017, 11:09 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Updated with review feedback.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/55895/diff/6-7/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by James Peach <jp...@apache.org>.
> On May 12, 2017, 5:13 p.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?
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?
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/#review174763
-----------------------------------------------------------
On May 9, 2017, 11:59 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55895/
> -----------------------------------------------------------
>
> (Updated May 9, 2017, 11:59 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/6/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> 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?
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. :)
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/#review174763
-----------------------------------------------------------
On May 9, 2017, 4:59 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55895/
> -----------------------------------------------------------
>
> (Updated May 9, 2017, 4:59 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/6/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> 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. :)
>
> Jiang Yan Xu wrote:
> 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.
I revised it to:
```
// Set both the hard and the soft limit to the same quota. Functionally
// all we need is the hard limit. The soft limit has no effect when it
// is the same as the hard limit but we set it for explicitness.
```
Let me know if it can be improved.
- 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
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> 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
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/#review174763
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/xfs/utils.hpp
Lines 64 (patched)
<https://reviews.apache.org/r/55895/#comment248003>
This could be private, see reasons below.
src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Line 62 (original), 62 (patched)
<https://reviews.apache.org/r/55895/#comment247993>
So I looked into this. You had to do this because of the **odr-use** rule and constexpr: http://en.cppreference.com/w/cpp/language/definition
This may be an arcane language rule but it led me to question why we needed it to be a `constexpr` member and why we needed to use it outside of the class. Looks like we don't?
Strictly speaking this `BasicBlocks` abstraction could be made a literal class, but we don't even have a use for that right now.
src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 157-158 (original), 155-156 (patched)
<https://reviews.apache.org/r/55895/#comment248007>
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?
src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 269-273 (original), 267-273 (patched)
<https://reviews.apache.org/r/55895/#comment248004>
So this is the only place `BasicBlocks::BASIC_BLOCK_SIZE` is used outside the class but the logic seems wrong. We used to "round down" like `quota.d_blk_hardlimit = limit.bytes() / BASIC_BLOCK_SIZE.bytes();` (which was probably bug). Now we round up so here we should actually only care about `if (limit == 0) {...}`?
Seems like all positive numbers are fine as limits? e.g., we should allow `limit == 1` if we allow `limit == 513`?
Since we don't need `BasicBlocks::BASIC_BLOCK_SIZE` here, we can make it private.
Even in some hypothetical scenarios where we needed to compare bytes and BasicBlocks, it should probably be `limit < BasicBlocks(1).bytes()` or we can make a constexpr that is of the type BasicBlocks?
- Jiang Yan Xu
On May 9, 2017, 4:59 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55895/
> -----------------------------------------------------------
>
> (Updated May 9, 2017, 4:59 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/6/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/
-----------------------------------------------------------
(Updated May 9, 2017, 11:59 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 (updated)
-----
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/6/
Changes: https://reviews.apache.org/r/55895/diff/5-6/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by James Peach <jp...@apache.org>.
> On May 9, 2017, 11:55 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp
> > Line 62 (original), 62 (patched)
> > <https://reviews.apache.org/r/55895/diff/5/?file=1703825#file1703825line62>
> >
> > This is unnecessary?
This comment is now on the `class BasicBlocks`.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/#review174391
-----------------------------------------------------------
On April 29, 2017, 5:41 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55895/
> -----------------------------------------------------------
>
> (Updated April 29, 2017, 5:41 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/5/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/#review174391
-----------------------------------------------------------
Fix it, then Ship it!
I can fix up the following the suggestions.
src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Line 62 (original), 62 (patched)
<https://reviews.apache.org/r/55895/#comment247526>
This is unnecessary?
src/tests/containerizer/xfs_quota_tests.cpp
Lines 950-951 (patched)
<https://reviews.apache.org/r/55895/#comment247527>
We can group the two statements and remove the redundant "// A partial block should round up."?
- Jiang Yan Xu
On April 29, 2017, 10:41 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55895/
> -----------------------------------------------------------
>
> (Updated April 29, 2017, 10:41 a.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/5/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/
-----------------------------------------------------------
(Updated April 29, 2017, 5:41 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 (updated)
-----
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/5/
Changes: https://reviews.apache.org/r/55895/diff/4-5/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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
>
>
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/
-----------------------------------------------------------
(Updated April 25, 2017, 10: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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/55895/diff/3-4/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 55895: Extract a BasicBlocks class for disk block
arithmetic.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/
-----------------------------------------------------------
(Updated April 24, 2017, 6:07 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Summary (updated)
-----------------
Extract a BasicBlocks class for disk block arithmetic.
Bugs: MESOS-5116
https://issues.apache.org/jira/browse/MESOS-5116
Repository: mesos
Description (updated)
-------
Extract a BasicBlocks class to handle disk blocks to clarify block-based
arithmetic in the XFS disk isolator.
Diffs (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/55895/diff/2-3/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 55895: Add zero byte disk resource support to XFS.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55895/
-----------------------------------------------------------
(Updated March 17, 2017, 9:57 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-5393
https://issues.apache.org/jira/browse/MESOS-5393
Repository: mesos
Description
-------
Support containers with zero byte disk resources in the XFS
isolator. This works by setting the quota to the minimum amount
(1 block), since the quota system defines 0 as unlimited.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/xfs/disk.cpp 40f1049358ad99d3f213289e36def81c580f07f3
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/2/
Changes: https://reviews.apache.org/r/55895/diff/1-2/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach