You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Avinash sridharan <av...@mesosphere.io> on 2016/01/26 07:31:14 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/
-----------------------------------------------------------
(Updated Jan. 26, 2016, 6:31 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 `NetClsHandleMgr` class.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 7ce5243ed7476a1cfd1371b5ce25b62dc07432db
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 0798de747627ccc45b01a2668e16693757dc69a8
Diff: https://reviews.apache.org/r/42587/diff/
Testing (updated)
-------
make
Thanks,
Avinash sridharan
Re: Review Request 42587: Implemented the `NetClsHandleManager` class.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 42587: Implemented the `NetClsHandleManager` class.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Feb. 3, 2016, 7:50 a.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, line 58
> > <https://reviews.apache.org/r/42587/diff/7/?file=1230632#file1230632line58>
> >
> > We generally leave a line before the comment and the TODO to correctly demarcate where the comment ends and where the TODO begins.
> >
> > ```
> > // comments
> > //
> > // TODO(asridharan):
> > //
> > ```
Done.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42587/#review117588
-----------------------------------------------------------
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
>
>
Re: Review Request 42587: Implemented the `NetClsHandleManager` class.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42587/#review117588
-----------------------------------------------------------
Primarily fly-by style comments.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 58)
<https://reviews.apache.org/r/42587/#comment178825>
We generally leave a line before the comment and the TODO to correctly demarcate where the comment ends and where the TODO begins.
```
// comments
//
// TODO(asridharan):
//
```
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 91)
<https://reviews.apache.org/r/42587/#comment178831>
What does `handles.size()` returns. I am assuming its an `unsigned in`t and hence should fail compilation with `-Wsign-compare` on GCC.
Why don't we just do `size_t count = 1`?
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 95)
<https://reviews.apache.org/r/42587/#comment178832>
Should fit on one line?
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 96)
<https://reviews.apache.org/r/42587/#comment178833>
I see that we name `NetClsHandles` as `handles`.
Why not name `NetClsHandle` as just `handle`?
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 120)
<https://reviews.apache.org/r/42587/#comment178826>
Remove period at the end.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 131)
<https://reviews.apache.org/r/42587/#comment178834>
How about simplifying this to:
```
if (condition) {
return Nothing();
}
return Error(...);
```
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 154)
<https://reviews.apache.org/r/42587/#comment178827>
Remove period at the end.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 165)
<https://reviews.apache.org/r/42587/#comment178828>
How about simplifying this to:
```
if (condition) {
return Nothing();
}
return Error(...);
```
- Anand Mazumdar
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
>
>
Re: Review Request 42587: Implemented the `NetClsHandleManager` class.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42587/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 9:11 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 `NetClsHandleManager` class.
Diffs (updated)
-----
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 `NetClsHandleManager` class.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42587/#review117655
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 65)
<https://reviews.apache.org/r/42587/#comment178923>
Why return Result here? Should that be Try?
- 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
>
>
Re: Review Request 42587: Implemented the `NetClsHandleManager` class.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
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.
Summary (updated)
-----------------
Implemented the `NetClsHandleManager` class.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description (updated)
-------
Implemented the `NetClsHandleManager` class.
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42587/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 12:44 a.m.)
Review request for mesos and Jie Yu.
Changes
-------
Rebased.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description
-------
Implemented the `NetClsHandleMgr` class.
Diffs (updated)
-----
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>.
-----------------------------------------------------------
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 (updated)
-----
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
>
>
Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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>.
-----------------------------------------------------------
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 (updated)
-----
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