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