You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/03/02 18:38:57 UTC
Re: Review Request 31276: Added cgroup memory pressure listening tests.

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


Feel free to chat with me on that!


src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/31276/#comment121467>

    Any reason this cannot be moved to MemoryTestHelper? For example:
    ```
    MemoryTestHelper helper;
    
    // This spawns the helper process and save the pid.
    ASSERT_SOME(helper.start());
    ```



src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/31276/#comment121479>

    This is still implicit, isn't it? You just moved the logic from TEST_F(...) to the test fixture. First of all, please move most of the logics to TEST_F(...). Also, as I suggested in the previous review, please introduce methods in MemoryTestHelper to hide those details. For example:
    ```
    ASSERT_TRUE(helper.started());
    ```



src/tests/memory_test_helper.hpp
<https://reviews.apache.org/r/31276/#comment121481>

    Please add comments for those public methods.



src/tests/memory_test_helper.hpp
<https://reviews.apache.org/r/31276/#comment121482>

    I don't think the inheritence is necessary. What do you want them? I would imagine MemoryTestHelper support several operations. For instance:
    ```
    helper.increaseUnlockedRSS(Megabytes(100));
    helper.inreasePageCache(Megabytes(1000));
    ...
    ```


- Jie Yu


On Feb. 25, 2015, 9:59 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31276/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2015, 9:59 p.m.)
> 
> 
> Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu.
> 
> 
> Bugs: mesos-2136
>     https://issues.apache.org/jira/browse/mesos-2136
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added cgroup memory pressure listening tests.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am f9e697547906bf82032734ee78135536ad832449 
>   src/tests/cgroups_tests.cpp 75c61aad80f894acb92a9752e8d1b6af70e5b9a6 
>   src/tests/memory_test_helper.hpp PRE-CREATION 
>   src/tests/memory_test_helper.cpp PRE-CREATION 
>   src/tests/memory_test_helper_main.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31276/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>