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




src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (lines 74 - 79)
<https://reviews.apache.org/r/42587/#comment178871>

    Can you move that helper to the cpp file as a file local helper? I don't 
see it being used in the header.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 55 - 64)
<https://reviews.apache.org/r/42587/#comment178885>

    I would suggest wrapping the comments in 70 char.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 69)
<https://reviews.apache.org/r/42587/#comment178874>

    Can we put 0x to `hexify`?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 74)
<https://reviews.apache.org/r/42587/#comment178881>

    This is not needed, right? You can just do:
    ```
    used[primary].set(0);
    ```



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 75)
<https://reviews.apache.org/r/42587/#comment178879>

    NOTE:



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 81 - 85)
<https://reviews.apache.org/r/42587/#comment178884>

    I would put this under else branch:
    ```
    if (!used.contains(primary)) {
      ...
    } else if (used[primary].all()) {
      return Error(
        "...");
    }
    ```



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 87 - 89)
<https://reviews.apache.org/r/42587/#comment178886>

    Wrap the comment in 70 char width.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 90 - 102)
<https://reviews.apache.org/r/42587/#comment178887>

    I would prefer a for loop here:
    ```
    for (int i = 1; i < used[primary].size(); i++) {
      if (!used[primary].test(i)) {
        used[primary].set(i);
        
        return NetClsHandle(primary, i);
      }
    }
    ```



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 104 - 107)
<https://reviews.apache.org/r/42587/#comment178888>

    This should be UNREACHABLE()



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 119)
<https://reviews.apache.org/r/42587/#comment178890>

    Please use explict check
    ```
    if (handle.secondary == 0)
    ```



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 120)
<https://reviews.apache.org/r/42587/#comment178889>

    Indentation.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 124 - 126)
<https://reviews.apache.org/r/42587/#comment178891>

    Ditto on my previous comments.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 129)
<https://reviews.apache.org/r/42587/#comment178892>

    No need for this.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 153)
<https://reviews.apache.org/r/42587/#comment178893>

    Ditto on explict check.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 154)
<https://reviews.apache.org/r/42587/#comment178894>

    Ditto on indentation.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 158)
<https://reviews.apache.org/r/42587/#comment178895>

    Remove extra space between return and Error.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 163)
<https://reviews.apache.org/r/42587/#comment178896>

    No need for this temp variable.


- Jie Yu


On Feb. 3, 2016, 6:46 a.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42587/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2016, 6:46 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
>     https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented the `NetClsHandleManager` class.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
> 
> Diff: https://reviews.apache.org/r/42587/diff/
> 
> 
> Testing
> -------
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Reply via email to