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

Ship it!



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 17)
<https://reviews.apache.org/r/42047/#comment175426>

    I would use `__CGROUPS_NET_CLS_ISOLATOR_HPP__` here. We should probably fix 
others as well (not in this patch).



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 20)
<https://reviews.apache.org/r/42047/#comment175458>

    Ditto. not needed. Please include <string> though because that's used in 
Info, which is privite to this class.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 22)
<https://reviews.apache.org/r/42047/#comment175453>

    The rule I used for includes is that if the parent class already have that 
symbol in its interface, we don't have to include it again in the derived class.
    
    Since 'Future' is already part of the interface MesosIsolatorProcess, we 
don't need to include future.hpp again here.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (lines 25 - 26)
<https://reviews.apache.org/r/42047/#comment175455>

    YOu don't need them.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 28)
<https://reviews.apache.org/r/42047/#comment175452>

    This is not needed.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 30)
<https://reviews.apache.org/r/42047/#comment175456>

    This is not needed as it's already in the interface of the parent class.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (lines 38 - 43)
<https://reviews.apache.org/r/42047/#comment175459>

    I prefer to use 70 char width if possible. But up to you.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 84)
<https://reviews.apache.org/r/42047/#comment175449>

    Let's put that below 'struct Info' right above 'hierarchy'.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 88)
<https://reviews.apache.org/r/42047/#comment175451>

    Do you still need `_containerId` here?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 96)
<https://reviews.apache.org/r/42047/#comment175448>

    Can we use Owned<Info> here?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 17 - 33)
<https://reviews.apache.org/r/42047/#comment175460>

    No need for all the headers here.


- Jie Yu


On Jan. 12, 2016, 6:26 a.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42047/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 6:26 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4262
>     https://issues.apache.org/jira/browse/MESOS-4262
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Specified the CgroupsNetClsIsolatorProcess class. This adds the ability to 
> isolate a mesos container using the net_cls cgroup subsystem.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 81afdc6c3e9b062efb181b2f92a9185bdd4acfb1 
>   src/Makefile.am 865926c5b46e42c5e29d3645a700c4ad20c1f11d 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42047/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Reply via email to