mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 31276: Added cgroup memory pressure listening tests.
Date Wed, 25 Mar 2015 21:24:36 GMT

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


Overall looks good. Let's sync on the test body.


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

    Can you set the limit in the test?



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

    Same as that in the memory isolator, put that into a hashmap? Therefore, you don't need
to use 'Option'.



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

    I would rather move those fields to each tests. They are temporary variables.



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

    This is a little unreliable and non-deterministic. Any reason why you want to call increaseRSS
multiple times?
    
    Can you increase the RSS once to a size right below 'limit' and keep reading the counter
until it shows non-zero (and set a time limit for that).



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

    I don't quite get this part :(



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

    Please add a comment saying that this is a blocking call.



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

    I won't mention the 1MB buffer in the comment. This is impl. detail. Just mention you'll
use a buffer and doing multiple writes. Also, don't mention the rounding because that's impl.
detail as well. You can add a TODO in the impl.



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126054>

    kill this. WE typically use lambda::function directly in the code base.



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment125998>

    ```
    return Error("Expect at least one argument");
    ```



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment125992>

    ```
    return Error("The first argument '" + tokens[1] +
                 "' is not a byte size");
    ```



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment125999>

    Since allocateRSS is only used here, please kill the helper funciton and move the logic
into increaseRSS



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126044>

    Move this down after validation is done.
    
    Also, s/memory/buffer/



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126045>

    Ditto. Please make it more informative.



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126048>

    s/bytes/size/



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126046>

    Ditto.



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126047>

    Why this is necessary?



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126049>

    NOTE: The child ...



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126051>

    Can you simply use this existing interface:
    ```
    inline Try<Nothing> write(int fd, const std::string& message)
    ```
    
    That means, instead of using a char[] buffer, use a string as the buffer. You can leverage
this string constructor:
    ```
    string (size_t n, char c);
    ```
    
    That means you don't need the helper function as well. Please move the logics down here.



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126056>

    No need to create a helper funciton for that. Please just move the hashmap initialization
to 'execute()' below.



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126059>

    YOu are not checking the result of getline. This is not a good programming practice. Any
reason not using:
    ```
    while (getline(cin, line) >= 0) {
      ....
    }
    
    if (!cin.eof()) {
      cerr << "" << endl;
      return 1;
    }
    
    return 0;
    ```



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126060>

    WHat if the token size is 0? It's good to be defensive.
    ```
    if (tokens.empty()) {
      cerr << "" << endl;
      return 1;
    }
    ```



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126061>

    Let's add a contains check first:
    ```
    if (!commands.contains(tokens[0])) {
      cerr << "" << endl;
      return 1;
    }
    
    Try<Nothing> result = commands[tokens[0]](tokens);
    ```



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126064>

    s/sizeof(char)/sizeof(STARTED)/



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126065>

    Any reason not using process::reap()?



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126069>

    You probably want to inject "\n" here, rather than below.



src/tests/memory_test_helper.cpp
<https://reviews.apache.org/r/31276/#comment126070>

    Again, sizeof(DONE)


- Jie Yu


On March 24, 2015, 10:15 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31276/
> -----------------------------------------------------------
> 
> (Updated March 24, 2015, 10:15 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 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb 
>   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
> 
>


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