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/02/03 07:46:33 UTC
Re: Review Request 42587: Implemented the `NetClsHandleManager` class.
-----------------------------------------------------------
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 `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
>
>