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 49813: Added stubs for the unified cgroups isolator.
Date Mon, 11 Jul 2016 22:48:53 GMT

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (line 44)
<https://reviews.apache.org/r/49813/#comment207165>

    I think for public interfaces, we use this style so that doxygen can pick up that. However,
I don't think we should expose this class directly. Therefore, let's use `//` here.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 85 - 88)
<https://reviews.apache.org/r/49813/#comment207167>

    Can we add this method later when we actually filling the impl.?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 91 - 94)
<https://reviews.apache.org/r/49813/#comment207168>

    Ditto. Can we introduce this method when we actually fill the impl.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 118 - 154)
<https://reviews.apache.org/r/49813/#comment207169>

    Ditto. Let's don't introduce those helpers in this method.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 158 - 160)
<https://reviews.apache.org/r/49813/#comment207166>

    Let's use `//` here.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 56 - 62)
<https://reviews.apache.org/r/49813/#comment207170>

    Please don't introduce those helper yet in this patch. Ditto for all others. Only keep
those virtual methods that's defined in the parent class.



src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp (lines 17 - 18)
<https://reviews.apache.org/r/49813/#comment207172>

    I would probably further split this patch into two:
    1) introduce the cgroups isolator
    2) introduce the subsystem
    
    It'll be easier to review.



src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp (lines 74 - 75)
<https://reviews.apache.org/r/49813/#comment207174>

    It's hard to tell why we need this function from this patch. That's the reason I suggested
that we should adding helpers as we need those in the impl.


- Jie Yu


On July 9, 2016, 10:16 a.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49813/
> -----------------------------------------------------------
> 
> (Updated July 9, 2016, 10:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added stubs for the unified cgroups isolator.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
>   src/Makefile.am 28dd15166937ed672f81be5a598df149b8ed4302 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49813/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> haosdent huang
> 
>


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