You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Paul Brett <pa...@twopensource.com> on 2015/06/09 00:26:18 UTC
Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/
-----------------------------------------------------------
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2332
https://issues.apache.org/jira/browse/MESOS-2332
Repository: mesos
Description
-------
Report per-container metrics for network bandwidth throttling to the slave.
Diffs
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
Diff: https://reviews.apache.org/r/35229/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Paul Brett <pa...@twopensource.com>.
> On June 9, 2015, 5:03 p.m., Cong Wang wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 124
> > <https://reviews.apache.org/r/35229/diff/1/?file=980896#file980896line124>
> >
> > TRAFFIC_CONTROL_STATISTICS is too generic, I don't see what it means from the name.
The name matches the protocol buffer message name in include/mesos/mesos.proto.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/#review87224
-----------------------------------------------------------
On June 9, 2015, 5:29 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35229/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 5:29 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2332
> https://issues.apache.org/jira/browse/MESOS-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Report per-container metrics for network bandwidth throttling to the slave.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
> src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
>
> Diff: https://reviews.apache.org/r/35229/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/#review87224
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/35229/#comment139547>
TRAFFIC_CONTROL_STATISTICS is too generic, I don't see what it means from the name.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/35229/#comment139546>
Hmm, doesn't isSome() imply !isError()?
- Cong Wang
On June 8, 2015, 10:26 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35229/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 10:26 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2332
> https://issues.apache.org/jira/browse/MESOS-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Report per-container metrics for network bandwidth throttling to the slave.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
> src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
>
> Diff: https://reviews.apache.org/r/35229/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/#review87098
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35225, 35229]
All tests passed.
- Mesos ReviewBot
On June 8, 2015, 10:26 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35229/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 10:26 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2332
> https://issues.apache.org/jira/browse/MESOS-2332
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Report per-container metrics for network bandwidth throttling to the slave.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
> src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
>
> Diff: https://reviews.apache.org/r/35229/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/#review88246
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp (line 683)
<https://reviews.apache.org/r/35229/#comment140669>
1 extra line please.
src/slave/containerizer/isolators/network/port_mapping.cpp (line 686)
<https://reviews.apache.org/r/35229/#comment140676>
How about s/CopyNetworkStatsToProtobuf/addTrafficControlStatistics/
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 688 - 689)
<https://reviews.apache.org/r/35229/#comment140672>
See my comments below.
'statistics' -> 'result'
'stats' -> 'statistics'
src/slave/containerizer/isolators/network/port_mapping.cpp (line 694)
<https://reviews.apache.org/r/35229/#comment140670>
Can you include `using namespace routing::queueing::statistics` so that we don't need to specify the prefix.
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 694 - 711)
<https://reviews.apache.org/r/35229/#comment140673>
```
// TODO: Use protobuf reflection here.
if (statistics.contains(BACKLOG)) {
tc->set_backlog(statistics[BACKLOG]);
}
...
```
src/slave/containerizer/isolators/network/port_mapping.cpp (line 713)
<https://reviews.apache.org/r/35229/#comment140668>
Extra line please.
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 858 - 859)
<https://reviews.apache.org/r/35229/#comment140667>
Can you pass in eth0_name as we did for PortMappingUpdate? `link::eth0` is pretty heavy wait (involing reading the routing table, etc.)
src/slave/containerizer/isolators/network/port_mapping.cpp (line 861)
<https://reviews.apache.org/r/35229/#comment140671>
Hum, we have too many 'stat' here in this file. How about
'statistics' -> 'result'
'stats' -> 'statistics'
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 863 - 866)
<https://reviews.apache.org/r/35229/#comment140674>
Do you want to print the error to stderr?
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 869 - 873)
<https://reviews.apache.org/r/35229/#comment140675>
Ditto on printing the error to stderr.
- Jie Yu
On June 16, 2015, 10 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35229/
> -----------------------------------------------------------
>
> (Updated June 16, 2015, 10 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2836
> https://issues.apache.org/jira/browse/MESOS-2836
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Report per-container metrics for network bandwidth throttling to the slave.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
> src/slave/containerizer/isolators/network/port_mapping.cpp e55e7b62bc29125458f9c0fb5477057ecc5a90df
>
> Diff: https://reviews.apache.org/r/35229/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/
-----------------------------------------------------------
(Updated June 17, 2015, 10:58 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2836
https://issues.apache.org/jira/browse/MESOS-2836
Repository: mesos
Description
-------
Report per-container metrics for network bandwidth throttling to the slave.
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 2a988b3b3a46aafa6e7f000d8741d01b546bca46
src/slave/containerizer/isolators/network/port_mapping.cpp f97e9608ebe1e72c83c22edb02600fd782db3bca
src/tests/port_mapping_tests.cpp 835a0f200dd5452adb3e553367ebde67414276f3
Diff: https://reviews.apache.org/r/35229/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Paul Brett <pa...@twopensource.com>.
> On June 17, 2015, 8:58 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 703
> > <https://reviews.apache.org/r/35229/diff/5/?file=986593#file986593line703>
> >
> > Please use
> > ```
> > statistics[BACKLOG];
> > ```
Since statistics is const, we need to use statistics.at(BACKLOG) etc.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/#review88275
-----------------------------------------------------------
On June 17, 2015, 10:58 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35229/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 10:58 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2836
> https://issues.apache.org/jira/browse/MESOS-2836
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Report per-container metrics for network bandwidth throttling to the slave.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 2a988b3b3a46aafa6e7f000d8741d01b546bca46
> src/slave/containerizer/isolators/network/port_mapping.cpp f97e9608ebe1e72c83c22edb02600fd782db3bca
> src/tests/port_mapping_tests.cpp 835a0f200dd5452adb3e553367ebde67414276f3
>
> Diff: https://reviews.apache.org/r/35229/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/#review88275
-----------------------------------------------------------
Ship it!
Please do a rebase.
src/slave/containerizer/isolators/network/port_mapping.cpp (line 696)
<https://reviews.apache.org/r/35229/#comment140710>
Could you please move that to the top of this file? Also, please use fully qualified namespace
```
using namespace routing::queueing::statistics;
```
src/slave/containerizer/isolators/network/port_mapping.cpp (line 703)
<https://reviews.apache.org/r/35229/#comment140711>
Please use
```
statistics[BACKLOG];
```
src/slave/containerizer/isolators/network/port_mapping.cpp (line 750)
<https://reviews.apache.org/r/35229/#comment140713>
s/results/result/ to be consistent with others.
src/slave/containerizer/isolators/network/port_mapping.cpp (line 869)
<https://reviews.apache.org/r/35229/#comment140712>
Can you create a temporary variable since it's used in more than one places:
```
const string& eth0 = flags.eth0_name.get();
```
src/slave/containerizer/isolators/network/port_mapping.cpp (line 879)
<https://reviews.apache.org/r/35229/#comment140714>
enter?
- Jie Yu
On June 17, 2015, 7:25 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35229/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 7:25 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2836
> https://issues.apache.org/jira/browse/MESOS-2836
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Report per-container metrics for network bandwidth throttling to the slave.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
> src/slave/containerizer/isolators/network/port_mapping.cpp e55e7b62bc29125458f9c0fb5477057ecc5a90df
>
> Diff: https://reviews.apache.org/r/35229/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/
-----------------------------------------------------------
(Updated June 17, 2015, 7:25 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2836
https://issues.apache.org/jira/browse/MESOS-2836
Repository: mesos
Description
-------
Report per-container metrics for network bandwidth throttling to the slave.
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
src/slave/containerizer/isolators/network/port_mapping.cpp e55e7b62bc29125458f9c0fb5477057ecc5a90df
Diff: https://reviews.apache.org/r/35229/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/
-----------------------------------------------------------
(Updated June 16, 2015, 10 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2836
https://issues.apache.org/jira/browse/MESOS-2836
Repository: mesos
Description
-------
Report per-container metrics for network bandwidth throttling to the slave.
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
src/slave/containerizer/isolators/network/port_mapping.cpp e55e7b62bc29125458f9c0fb5477057ecc5a90df
Diff: https://reviews.apache.org/r/35229/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/#review87485
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.hpp
<https://reviews.apache.org/r/35229/#comment139820>
Can you make those constexpr as well? Thanks!
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/35229/#comment139825>
Hum, this is my fault. But given that more and more stats are being added, we probably should construct a ResourceStatistics message in 'execute()' and stringify it to JSON and print it.
In the caller, we can parse it into ResourceStatistics and use MergeFrom to merge them?
- Jie Yu
On June 9, 2015, 9:56 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35229/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 9:56 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Bugs: MESOS-2836
> https://issues.apache.org/jira/browse/MESOS-2836
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Report per-container metrics for network bandwidth throttling to the slave.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
> src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
>
> Diff: https://reviews.apache.org/r/35229/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/
-----------------------------------------------------------
(Updated June 9, 2015, 9:56 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2836
https://issues.apache.org/jira/browse/MESOS-2836
Repository: mesos
Description
-------
Report per-container metrics for network bandwidth throttling to the slave.
Diffs
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
Diff: https://reviews.apache.org/r/35229/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/
-----------------------------------------------------------
(Updated June 9, 2015, 6:09 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
Switch to using constexpr for strings.
Bugs: MESOS-2332
https://issues.apache.org/jira/browse/MESOS-2332
Repository: mesos
Description
-------
Report per-container metrics for network bandwidth throttling to the slave.
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
Diff: https://reviews.apache.org/r/35229/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 35229: Report per-container metrics for network
bandwidth throttling to the slave.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35229/
-----------------------------------------------------------
(Updated June 9, 2015, 5:29 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
Address reviewer feedback
Bugs: MESOS-2332
https://issues.apache.org/jira/browse/MESOS-2332
Repository: mesos
Description
-------
Report per-container metrics for network bandwidth throttling to the slave.
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc
src/slave/containerizer/isolators/network/port_mapping.cpp d2da1a4e96baeac7d1af9a5468f90c2e4c1cb50f
Diff: https://reviews.apache.org/r/35229/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett