Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@mesos.apache.org
Content-Type: multipart/alternative;
 boundary="===============9137019968166813556=="
MIME-Version: 1.0
Subject: Re: Review Request 30545: cgroups: added support to listen on memory
 pressures.
From: "Jie Yu" <yujie.jay@gmail.com>
To: "Dominic Hamon" <dhamon@twopensource.com>, "Jie Yu" <yujie.jay@gmail.com>,
 "Ian Downes" <ian.downes@gmail.com>
Cc: "mesos" <dev@mesos.apache.org>, "Chi Zhang" <chzhcn88@gmail.com>
Date: Sat, 21 Feb 2015 01:27:02 -0000
Message-ID: <20150221012702.21353.44858@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Jie Yu" <noreply@reviews.apache.org>
References: <20150220213713.21353.50988@reviews.apache.org>
In-Reply-To: <20150220213713.21353.50988@reviews.apache.org>
Reply-To: "Jie Yu" <yujie.jay@gmail.com>

--===============9137019968166813556==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


The code looks great! Given that the tests are quite large. Can you split this patch and I'll give a shipit for the code.

For tests, I need to do a pass next week. Thanks! THis is great, Chi!


src/linux/cgroups.hpp
<https://reviews.apache.org/r/30545/#comment119660>

    Any reason this is not a static method of pressure::Listener?


src/linux/cgroups.hpp
<https://reviews.apache.org/r/30545/#comment119661>

    Well, you don't need this friend declaration if 'create' is a static member funciton, right?


src/linux/cgroups.cpp
<https://reviews.apache.org/r/30545/#comment119664>

    Look like only one place is using this constant. Sugguest killing it.


src/linux/cgroups.cpp
<https://reviews.apache.org/r/30545/#comment119663>

    I think we indent 2 spaces for switch 'case'.


src/linux/cgroups.cpp
<https://reviews.apache.org/r/30545/#comment119662>

    I think here should be able to do:
    ```
      : coutner_(0),
        error(None()),
        process(new event::Listener(
            hierarchy,
            cgroup,
            "memory.pressure_level",
            stringify(level))) {}
    ```


src/linux/cgroups.cpp
<https://reviews.apache.org/r/30545/#comment119668>

    Remove the tailing period.


src/linux/cgroups.cpp
<https://reviews.apache.org/r/30545/#comment119669>

    We don't use std:: prefix in cpp files. Please remove them (do a sweep in this file).


- Jie Yu


On Feb. 20, 2015, 9:37 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30545/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2015, 9:37 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
> -------
> 
> cgroups: added support to listen on memory pressures.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am d372404d84c9ac0bc49da7407ad366701b9586a6 
>   src/linux/cgroups.hpp e07772fec11b9bb73a44c54326f24d7ee1e8a64b 
>   src/linux/cgroups.cpp a307e27c5840270e28cced2d5aeb90c6679bff1d 
>   src/tests/cgroups_tests.cpp 9d50a47c939f5f136c85fdcc555459512c6f2259 
>   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/30545/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>


--===============9137019968166813556==--