You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2014/12/15 04:26:33 UTC
Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/
-----------------------------------------------------------
Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
Repository: mesos-git
Description
-------
While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
Diffs
-----
include/mesos/mesos.proto 540071d
src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
src/slave/flags.hpp 670997d
src/tests/port_mapping_tests.cpp eb82993
Diff: https://reviews.apache.org/r/29033/diff/
Testing
-------
expanded a test case to test PortMappingStatistics.
Thanks,
Chi Zhang
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
> On Dec. 15, 2014, 6:02 p.m., Dominic Hamon wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 538
> > <https://reviews.apache.org/r/29033/diff/1/?file=791613#file791613line538>
> >
> > just because we can't read /proc/net/sockstat, we might still want to do the expensive socket stats below. if this was a different method you could keep this logic.
When the RTT patch was initially developed, I used a 'get-as-much-stats-as-possible' approach but then reverted back to 'return-upon-error' for simplicity.
I will use @jieyu's suggestion to have both 'summary' and 'details' controllable by flags, but will still keep a 'no-error-allowed' guideline for ::usage.
> On Dec. 15, 2014, 6:02 p.m., Dominic Hamon wrote:
> > src/tests/port_mapping_tests.cpp, line 1517
> > <https://reviews.apache.org/r/29033/diff/1/?file=791615#file791615line1517>
> >
> > should be a timeout on here
I will keep this one as is for this patch. There are multiple occurences of that busy loop in this file so I will do another patch just for this change.
> On Dec. 15, 2014, 6:02 p.m., Dominic Hamon wrote:
> > src/tests/port_mapping_tests.cpp, line 1514
> > <https://reviews.apache.org/r/29033/diff/1/?file=791615#file791615line1514>
> >
> > do you really need 5 seconds? this is overhead on every test run.
5 or more seconds is an overkill. IIRC, this pattern has been repeated in the test code base where ::usage is exercised. I haven't seen a noticable delay as it should just return after no more than a few calls to ::usage.
I will keep this one as it is for this patch, and do another one to go through all of them to adjust the timeout value.
> On Dec. 15, 2014, 6:02 p.m., Dominic Hamon wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 547
> > <https://reviews.apache.org/r/29033/diff/1/?file=791613#file791613line547>
> >
> > why do you need to find (which is a linear search) twice?
> >
> > instead, just loop over the tokens and check for both "inuse" or "tw" and then numify the next token. then you only search through the tokens once.
I changed the parsing logic to do one interation only. The code is a tad harder to read.
> On Dec. 15, 2014, 6:02 p.m., Dominic Hamon wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 579
> > <https://reviews.apache.org/r/29033/diff/1/?file=791613#file791613line579>
> >
> > consider separating this into a separate function for independent testing and code readability.
split into 2 functions and added tests.
> On Dec. 15, 2014, 6:02 p.m., Dominic Hamon wrote:
> > src/tests/port_mapping_tests.cpp, line 1418
> > <https://reviews.apache.org/r/29033/diff/1/?file=791615#file791615line1418>
> >
> > you need a test that ensures this gives the right result when this is false.
tested all combinations of the flags.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review65095
-----------------------------------------------------------
On Dec. 23, 2014, 7:51 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2014, 7:51 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review65095
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108033>
just because we can't read /proc/net/sockstat, we might still want to do the expensive socket stats below. if this was a different method you could keep this logic.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108034>
this is going to go through the entire output once and then you're going to go through each line. this could be much faster if you just read the string up to each '\n' once.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108036>
const_iterator
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108037>
why is this initialization separate to the declaration?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108035>
why do you need to find (which is a linear search) twice?
instead, just loop over the tokens and check for both "inuse" or "tw" and then numify the next token. then you only search through the tokens once.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108032>
consider separating this into a separate function for independent testing and code readability.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment108038>
you need a test that ensures this gives the right result when this is false.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment108040>
do you really need 5 seconds? this is overhead on every test run.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment108039>
should be a timeout on here
- Dominic Hamon
On Dec. 14, 2014, 7:26 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2014, 7:26 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68789
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29033]
All tests passed.
- Mesos ReviewBot
On Jan. 20, 2015, 8:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 8:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68779
-----------------------------------------------------------
Ship it!
- Jie Yu
On Jan. 20, 2015, 8:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 8:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 21, 2015, 12:02 a.m., Ben Mahler wrote:
> > src/slave/flags.hpp, lines 379-389
> > <https://reviews.apache.org/r/29033/diff/7/?file=827142#file827142line379>
> >
> > It would be great to clarify in the help messages that these flags are only relevant for the 'network/port_mapping' isolator. Ditto for the other network flags.
>
> Chi Zhang wrote:
> I will have a patch for all network/port_mapping related flags.
In fact, I've already done that when I committed this patch.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68811
-----------------------------------------------------------
On Jan. 20, 2015, 8:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 8:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 21, 2015, 12:02 a.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 492-495
> > <https://reviews.apache.org/r/29033/diff/7/?file=827139#file827139line492>
> >
> > These comments just seem to repeat what's in the name of the field? Do they add any clarity or can we pull them out?
Chi, can you have a patch to remove those comments? THanks!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68811
-----------------------------------------------------------
On Jan. 20, 2015, 8:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 8:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
> On Jan. 21, 2015, 12:02 a.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 492-495
> > <https://reviews.apache.org/r/29033/diff/7/?file=827139#file827139line492>
> >
> > These comments just seem to repeat what's in the name of the field? Do they add any clarity or can we pull them out?
>
> Jie Yu wrote:
> Chi, can you have a patch to remove those comments? THanks!
ack.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68811
-----------------------------------------------------------
On Jan. 20, 2015, 8:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 8:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
> On Jan. 21, 2015, 12:02 a.m., Ben Mahler wrote:
> > src/slave/flags.hpp, lines 379-389
> > <https://reviews.apache.org/r/29033/diff/7/?file=827142#file827142line379>
> >
> > It would be great to clarify in the help messages that these flags are only relevant for the 'network/port_mapping' isolator. Ditto for the other network flags.
I will have a patch for all network/port_mapping related flags.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68811
-----------------------------------------------------------
On Jan. 20, 2015, 8:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 8:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68811
-----------------------------------------------------------
Just some minor comments from the perspective of a user (CLI and protobuf API).
include/mesos/mesos.proto
<https://reviews.apache.org/r/29033/#comment113289>
These comments just seem to repeat what's in the name of the field? Do they add any clarity or can we pull them out?
src/slave/flags.hpp
<https://reviews.apache.org/r/29033/#comment113288>
It would be great to clarify in the help messages that these flags are only relevant for the 'network/port_mapping' isolator. Ditto for the other network flags.
- Ben Mahler
On Jan. 20, 2015, 8:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 8:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 20, 2015, 9:01 p.m., Dominic Hamon wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 570
> > <https://reviews.apache.org/r/29033/diff/7/?file=827141#file827141line570>
> >
> > nit: the pattern throughout is cerr/continue. why not do the same here and lose the else:
> >
> > if (inuse.isError()) {
> > cerr << "...";
> > continue;
> > }
> > object.values["..."] = inuse.get();
Good catch. I've corrected it in the commit.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68780
-----------------------------------------------------------
On Jan. 20, 2015, 8:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 8:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68780
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment113218>
nit: the pattern throughout is cerr/continue. why not do the same here and lose the else:
if (inuse.isError()) {
cerr << "...";
continue;
}
object.values["..."] = inuse.get();
- Dominic Hamon
On Jan. 20, 2015, 12:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 12:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review68783
-----------------------------------------------------------
Ship it!
Minor comment nits.
src/slave/flags.hpp
<https://reviews.apache.org/r/29033/#comment113219>
s/summaray/summary/
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment113220>
s/set-up/setup/
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment113221>
s/combinations of the flags/flag combinations/
- Ian Downes
On Jan. 20, 2015, 12:21 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 12:21 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 859aa2f
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 33a0cb6
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/
-----------------------------------------------------------
(Updated Jan. 20, 2015, 8:21 p.m.)
Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
rebased master.
Repository: mesos-git
Description
-------
While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
Diffs (updated)
-----
include/mesos/mesos.proto 859aa2f
src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
src/slave/flags.hpp 33a0cb6
src/tests/port_mapping_tests.cpp d57d3e6
Diff: https://reviews.apache.org/r/29033/diff/
Testing
-------
expanded a test case to test PortMappingStatistics.
Thanks,
Chi Zhang
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/
-----------------------------------------------------------
(Updated Jan. 14, 2015, 2:06 a.m.)
Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
Repository: mesos-git
Description
-------
While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
Diffs (updated)
-----
include/mesos/mesos.proto 5007c0c
src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
src/slave/flags.hpp f1b8dfb
src/tests/port_mapping_tests.cpp d57d3e6
Diff: https://reviews.apache.org/r/29033/diff/
Testing
-------
expanded a test case to test PortMappingStatistics.
Thanks,
Chi Zhang
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review67368
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29033]
All tests passed.
- Mesos ReviewBot
On Jan. 9, 2015, 2:55 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 2:55 a.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 6c9f5d3
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp f1b8dfb
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
> On Jan. 14, 2015, 1:09 a.m., Jie Yu wrote:
> > src/tests/port_mapping_tests.cpp, lines 1631-1632
> > <https://reviews.apache.org/r/29033/diff/5/?file=813956#file813956line1631>
> >
> > Is it guaranteed that the connective will be still active? If not, I would rather not introduce those checks as it could cause flackyness.
I want to keep these tests because they gurantee that I have done the control structure based on flags right in `PortMappingStatistics::execute`. `PortMappingIsolator::__usage` could then be simplied to merely retriving all the results. Having these checks here gives more faith there.
As for the performance, there is only one test connection here so both 'summary' and 'details' should return in no time.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review67989
-----------------------------------------------------------
On Jan. 14, 2015, 2:06 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2015, 2:06 a.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5007c0c
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp f1b8dfb
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review67989
-----------------------------------------------------------
Much better!
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment112105>
Do you still need this header?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment112109>
Kill this.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment112110>
Kill this.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment112114>
size_t i = 0;
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment112116>
we don't use compare. Please just use ==
if (tokens[i] == "inuse")
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment112118>
This is a bit wiered. What if in the future we want to introduce more flags? Can you just wrap the 'details' code path in the if block?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment112113>
You probably do wanna proceed if both flags are off, right?
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment112126>
Is it guaranteed that the connective will be still active? If not, I would rather not introduce those checks as it could cause flackyness.
- Jie Yu
On Jan. 9, 2015, 2:55 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 2:55 a.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 6c9f5d3
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp f1b8dfb
> src/tests/port_mapping_tests.cpp d57d3e6
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/
-----------------------------------------------------------
(Updated Jan. 9, 2015, 2:55 a.m.)
Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description
-------
While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
Diffs (updated)
-----
include/mesos/mesos.proto 6c9f5d3
src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
src/slave/flags.hpp f1b8dfb
src/tests/port_mapping_tests.cpp d57d3e6
Diff: https://reviews.apache.org/r/29033/diff/
Testing
-------
expanded a test case to test PortMappingStatistics.
Thanks,
Chi Zhang
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review66855
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/29033/#comment110519>
// Total number of active tcp connections in a container.
include/mesos/mesos.proto
<https://reviews.apache.org/r/29033/#comment110520>
Total number of ... in a container.
include/mesos/mesos.proto
<https://reviews.apache.org/r/29033/#comment110521>
s/tw/timed_wait/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110522>
static please.
Also, we seldom use non const references. Please use the following signature:
Try<JSON::Object> getTCPSocketsCount();
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110538>
Kill this. See my comments below.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110523>
We don't use this style. Please use two lines:
// ...
size_t index = 0;
size_t found = 0;
// ...
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110536>
This is a little hard to read and follow, and we seldom use std iterators or non const references. How about the following.
```
foreach (const string& line, strings::tokenize(value.get(), "\n")) {
if (!strings::startsWith(line, "TCP")) {
continue;
}
vector<string> tokens = strings::tokenize(line, " ");
for (int i = 0; i < tokens.size(); i++) {
if (tokens[i] == "inuse") {
if (i + 1 >= tokens.size()) {
return Error("Unexpected output from /proc/net/sockstat");
}
Try<size_t> inuse = numify<size_t>(tokens[i+1]);
if (inuse.isError()) {
return Error(...);
}
object.values["..."] = inuse.get();
} else if (tokens[i] == "tw") {
// Similar to above.
// ...
}
}
}
return object;
```
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110541>
Ditto above.
Actually, given that you need to merge JSON::Object below. I would suggest simply just kill these helper functions and put all logics in PortMappingStatistics::execute so that people can read your code without jump back and forth.
- Jie Yu
On Jan. 6, 2015, 12:01 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 12:01 a.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review66769
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29033]
All tests passed.
- Mesos ReviewBot
On Jan. 6, 2015, 12:01 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 12:01 a.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/
-----------------------------------------------------------
(Updated Jan. 6, 2015, 12:01 a.m.)
Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description
-------
While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
Diffs (updated)
-----
include/mesos/mesos.proto 540071d
src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
src/slave/flags.hpp 670997d
src/tests/port_mapping_tests.cpp eb82993
Diff: https://reviews.apache.org/r/29033/diff/
Testing
-------
expanded a test case to test PortMappingStatistics.
Thanks,
Chi Zhang
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review66732
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29033]
All tests passed.
- Mesos ReviewBot
On Jan. 5, 2015, 8:51 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 8:51 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review66745
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.hpp
<https://reviews.apache.org/r/29033/#comment110352>
No need. See my comments below.
src/slave/containerizer/isolators/network/port_mapping.hpp
<https://reviews.apache.org/r/29033/#comment110353>
Ditto.
src/slave/containerizer/isolators/network/port_mapping.hpp
<https://reviews.apache.org/r/29033/#comment110351>
I don't think those functions need to be exposed (given that the tests use these interfaces are not necessary, please see my comments below).
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110354>
Please make it static.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110355>
Ditto.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110356>
Please insert a blank line after that. Here and everywhere else.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment110357>
s/portMappingStatisticsHelper/statisticsHelper/
Since the class is named as PortMapping...
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment110358>
I don't think those tests are necessary since the code should have been tested in the following test.
- Jie Yu
On Jan. 5, 2015, 8:51 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 8:51 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/
-----------------------------------------------------------
(Updated Jan. 5, 2015, 8:51 p.m.)
Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description
-------
While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
Diffs (updated)
-----
include/mesos/mesos.proto 540071d
src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
src/slave/flags.hpp 670997d
src/tests/port_mapping_tests.cpp eb82993
Diff: https://reviews.apache.org/r/29033/diff/
Testing
-------
expanded a test case to test PortMappingStatistics.
Thanks,
Chi Zhang
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review65971
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29033]
All tests passed.
- Mesos ReviewBot
On Dec. 23, 2014, 7:51 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2014, 7:51 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review66669
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment110243>
if (out.get().empty()) {
...
- Dominic Hamon
On Dec. 23, 2014, 11:51 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2014, 11:51 a.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/
-----------------------------------------------------------
(Updated Dec. 23, 2014, 7:51 p.m.)
Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description
-------
While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
Diffs (updated)
-----
include/mesos/mesos.proto 540071d
src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
src/slave/flags.hpp 670997d
src/tests/port_mapping_tests.cpp eb82993
Diff: https://reviews.apache.org/r/29033/diff/
Testing
-------
expanded a test case to test PortMappingStatistics.
Thanks,
Chi Zhang
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
> On Dec. 15, 2014, 6:45 p.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 543
> > <https://reviews.apache.org/r/29033/diff/1/?file=791613#file791613line543>
> >
> > what about continuing if !startsWith(line, "TCP") to reduce the nesting?
Fixed as suggested.
Was thinking later on we'd do:
if (TCP) {
...
} else if (UDP) {
...
}
> On Dec. 15, 2014, 6:45 p.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 525
> > <https://reviews.apache.org/r/29033/diff/1/?file=791613#file791613line525>
> >
> > Can you assume the output format stable? does it depend on anything, e.g., if ipv6 is enabled?
> >
> > Can you add a note here why reading `/proc/net/sockstat` is not expensive.
Left a note for /proc/net/sockstat, the IPv6 counter part.
There is little documentation of this proc file (not in the proc man page). I think it's fine since usually kernel keeps it compatibile.
In the code, warnings are reported parsing the output, but program wouldn't abort but continue -- at least this shouldn't crash.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review65099
-----------------------------------------------------------
On Dec. 23, 2014, 7:51 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2014, 7:51 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review65099
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108047>
Can you assume the output format stable? does it depend on anything, e.g., if ipv6 is enabled?
Can you add a note here why reading `/proc/net/sockstat` is not expensive.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108050>
what about continuing if !startsWith(line, "TCP") to reduce the nesting?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108051>
what about splitting on the ":" since that denotes the differences between lines
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108045>
kill newline
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/29033/#comment108053>
you should be able to at least check for an active tcp connection as well?
- Ian Downes
On Dec. 14, 2014, 7:26 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2014, 7:26 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review65071
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29033]
All tests passed.
- Mesos ReviewBot
On Dec. 15, 2014, 3:26 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2014, 3:26 a.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Chi Zhang <ch...@gmail.com>.
> On Dec. 15, 2014, 6:51 p.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 483
> > <https://reviews.apache.org/r/29033/diff/1/?file=791611#file791611line483>
> >
> > Ditto for removing the comments.
> >
> > s/net_tcp_total_tw_connections/net_tcp_time_wait_connections/
changed the variable names, but I kept the comments -- no harm to have some documentation where they are first introduced?
> On Dec. 15, 2014, 6:51 p.m., Jie Yu wrote:
> > src/slave/flags.hpp, lines 375-377
> > <https://reviews.apache.org/r/29033/diff/1/?file=791614#file791614line375>
> >
> > I would suggest using two separate flags:
> >
> > ```
> > network_enable_socket_statistics_details
> > network_enable_socket_statistics_summary
> > ```
> >
> > The benefit is that you can choose to turn off all of them (which we will use initially).
good point. fixed.
> On Dec. 15, 2014, 6:51 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, lines 2202-2227
> > <https://reviews.apache.org/r/29033/diff/1/?file=791613#file791613line2202>
> >
> > Why doing this change? I would suggest keep the original code because it implies a ALL-OR-NOTHING semantics for those rtt statistics.
1. I intended for only `PortMappingStatistics::execute` to control its stats collection / output though the use of flags.
2. `::__usage` can then be simplied try to just get all the results, ignoring the flags, otherwise, the condition logic based on the flags needs to be repeated here as well, e.g, to do all_or_nothing for the RTT values one needs checking the 'expensive' flag is true.
3. a new test has been added that keeps the all-or-nothing check.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review65101
-----------------------------------------------------------
On Dec. 23, 2014, 7:51 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2014, 7:51 p.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 29033: PortMappingIsolator: added metrics for the
numbers of active and TIME_WAIT tcp connections.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29033/#review65101
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/29033/#comment108043>
s/net_tcp_total_active_connections/net_tcp_active_connections/
Also, no need for the comments because it's pretty clear from the variable name.
include/mesos/mesos.proto
<https://reviews.apache.org/r/29033/#comment108044>
Ditto for removing the comments.
s/net_tcp_total_tw_connections/net_tcp_time_wait_connections/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108055>
Consider using tokenize instead of split.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108056>
Ditto. Consider using tokenize instead of split.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/29033/#comment108057>
Why doing this change? I would suggest keep the original code because it implies a ALL-OR-NOTHING semantics for those rtt statistics.
src/slave/flags.hpp
<https://reviews.apache.org/r/29033/#comment108049>
I would suggest using two separate flags:
```
network_enable_socket_statistics_details
network_enable_socket_statistics_summary
```
The benefit is that you can choose to turn off all of them (which we will use initially).
- Jie Yu
On Dec. 15, 2014, 3:26 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29033/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2014, 3:26 a.m.)
>
>
> Review request for mesos, Dominic Hamon, Ian Downes, Jie Yu, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> While we are still working to make RTT less expensive to get, expose total number of tcp connections in use and in TIME_WAIT.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 540071d
> src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352
> src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336
> src/slave/flags.hpp 670997d
> src/tests/port_mapping_tests.cpp eb82993
>
> Diff: https://reviews.apache.org/r/29033/diff/
>
>
> Testing
> -------
>
> expanded a test case to test PortMappingStatistics.
>
>
> Thanks,
>
> Chi Zhang
>
>