mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 29033: PortMappingIsolator: added metrics for the numbers of active and TIME_WAIT tcp connections.
Date Tue, 06 Jan 2015 17:32:48 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message