You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2016/02/01 23:46:08 UTC
Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42587/#review117230
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (lines 137 - 139)
<https://reviews.apache.org/r/42587/#comment178453>
Hum, having two data fields here looks a little confusing to me. How about just merging them:
```
// A map for used handles: primary -> bitset for secondary.
hashmap<uint16_t, bitset<0xffff>> used;
```
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 53)
<https://reviews.apache.org/r/42587/#comment178358>
Please insert a blank line above.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 53 - 62)
<https://reviews.apache.org/r/42587/#comment178433>
This is a simple 'contains' check. For readability, I would rather inline this instead of creating a helper method.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 73 - 78)
<https://reviews.apache.org/r/42587/#comment178432>
I got confused here. minor is a uint16 value. Why this check is necessary?
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 82 - 96)
<https://reviews.apache.org/r/42587/#comment178456>
For testin, I would probably just expose a 'isUsed' method:
```
bool isUsed(const NetClsHandle& handle);
```
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 102 - 105)
<https://reviews.apache.org/r/42587/#comment178434>
See my comments above. I would rather have the 'contains' check here.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 113)
<https://reviews.apache.org/r/42587/#comment178455>
why minorHandle -1 here?
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 114 - 116)
<https://reviews.apache.org/r/42587/#comment178454>
Fix error message here (indentation and '+')
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 154)
<https://reviews.apache.org/r/42587/#comment178458>
I would simply start from position 0 and find the first unset bit. We won't have a lot of containers (O(100) as most). So I won't be too worried about the performance here. Add a TODO to do it more efficiently should be sufficient.
- Jie Yu
On Jan. 31, 2016, 5:56 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42587/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2016, 5:56 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented the `NetClsHandleMgr` 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
>
>
Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, lines 137-139
> > <https://reviews.apache.org/r/42587/diff/4/?file=1227354#file1227354line137>
> >
> > Hum, having two data fields here looks a little confusing to me. How about just merging them:
> >
> > ```
> > // A map for used handles: primary -> bitset for secondary.
> > hashmap<uint16_t, bitset<0xffff>> used;
> > ```
Used a typedef for bitset<0xffff+1> to improve readability.
> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 73-78
> > <https://reviews.apache.org/r/42587/diff/4/?file=1227355#file1227355line73>
> >
> > I got confused here. minor is a uint16 value. Why this check is necessary?
Removed this validation. Always set the first element of the bitmask when initializing it, so no longer need the check on the secondary handles.
> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 82-96
> > <https://reviews.apache.org/r/42587/diff/4/?file=1227355#file1227355line82>
> >
> > For testin, I would probably just expose a 'isUsed' method:
> > ```
> > bool isUsed(const NetClsHandle& handle);
> > ```
Removed usage(). Will introduce isUsed with the unit-test patch.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42587/#review117230
-----------------------------------------------------------
On Feb. 2, 2016, 6:16 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42587/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 6:16 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented the `NetClsHandleMgr` 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
>
>