> 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 > >
