mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ian Downes" <ian.dow...@gmail.com>
Subject Re: Review Request 38117: Export per container SNMP statistics
Date Fri, 16 Oct 2015 18:00:11 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38117/#review102933
-----------------------------------------------------------



include/mesos/mesos.proto (lines 702 - 720)
<https://reviews.apache.org/r/38117/#comment160766>

    Are the statistics signed? If not, suggest using uint64 type.



src/slave/containerizer/isolators/network/port_mapping.cpp (line 1139)
<https://reviews.apache.org/r/38117/#comment160779>

    No need to store all statistics until the end. Key/value lines for a set of statistics
are paired so you can add*Statistics after processing each line pair. Then you just need a
scoped {{hashmap<string, int64_t> statistics}}.



src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1140 - 1141)
<https://reviews.apache.org/r/38117/#comment160778>

    Clearer to just declare these inside the loop scope (but my suggestions below obviate
them anyway).



src/slave/containerizer/isolators/network/port_mapping.cpp (line 1142)
<https://reviews.apache.org/r/38117/#comment160777>

    you don't care about the actual line number, just that you want to toggle between "key"
and "value" lines, suggest something like "bool isKeyLine" and toggle that?



src/slave/containerizer/isolators/network/port_mapping.cpp (line 1144)
<https://reviews.apache.org/r/38117/#comment160770>

    Suggest splitting first on ":" to get the statistics type and keys/values, then tokenize
on " ". You can also validate the type then as well.



src/slave/containerizer/isolators/network/port_mapping.cpp (line 1146)
<https://reviews.apache.org/r/38117/#comment160774>

    If you split first on ":" then you don't need a separate keys vector.



src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1150 - 1159)
<https://reviews.apache.org/r/38117/#comment160775>

    Assuming you've got the type from splitting on ":" and then you've got the keys, you can
directly insert val into a statistics hashmap after successfully numifying it, i.e., no need
for an intermediate values vector.



src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1161 - 1165)
<https://reviews.apache.org/r/38117/#comment160780>

    This is an unnecessary mix of indexing and foreach looping. Comments above make this unnecessary
but just FYI.



src/slave/containerizer/isolators/network/port_mapping.cpp (line 1162)
<https://reviews.apache.org/r/38117/#comment160783>

    This should be a {{const string&}} (if kept).



src/slave/containerizer/isolators/network/port_mapping.cpp (line 1169)
<https://reviews.apache.org/r/38117/#comment160782>

    Toggle line type here.



src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1171 - 1174)
<https://reviews.apache.org/r/38117/#comment160781>

    Do these inside the parsing loop as you parse each line pair for a statistics type.


- Ian Downes


On Sept. 8, 2015, 2:12 p.m., Cong Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38117/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2015, 2:12 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Paul Brett, and Vinod Kone.
> 
> 
> Bugs: MESOS-3365
>     https://issues.apache.org/jira/browse/MESOS-3365
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> These stats are those we get by `netstat -s`, they are important for diagnose networking
issues.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   src/slave/containerizer/isolators/network/port_mapping.hpp 4bca0b81bf69fb4cd75e05aacd02d3e818e32d09

>   src/slave/containerizer/isolators/network/port_mapping.cpp 34ba2294b0bd7d57aa9de073692a2ea8ec62681d

>   src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c 
>   src/slave/flags.cpp 7539441c685828027db07173e62a4e5fc1e8b54d 
> 
> Diff: https://reviews.apache.org/r/38117/diff/
> 
> 
> Testing
> -------
> 
> Manual tests
> 
> 
> Thanks,
> 
> Cong Wang
> 
>


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