mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 42047: Specified the CgroupsNetClsIsolatorProcess class. This adds the ability to isolate a mesos container using the net_cls cgroup subsystem.
Date Thu, 14 Jan 2016 23:05:27 GMT

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


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