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


maybe pull out the improvment part to existing code. makes that part quicker to 
go in and this one shorter and easier to review.


include/mesos/mesos.proto
<https://reviews.apache.org/r/32660/#comment131993>

    let's avoid a new term network rate limiter and just use BW capping?



include/mesos/mesos.proto
<https://reviews.apache.org/r/32660/#comment131994>

    maybe make it clear that delayed maps to capping; dropped maps to NIC 
capacity exceeded?



include/mesos/mesos.proto
<https://reviews.apache.org/r/32660/#comment131995>

    Maybe give a reference link to where these are introduced?
    
    I am a bit confused about backlog and qlen



include/mesos/mesos.proto
<https://reviews.apache.org/r/32660/#comment131996>

    required makes it hard to update a protobuf:
    
    https://developers.google.com/protocol-buffers/docs/proto#updating



include/mesos/mesos.proto
<https://reviews.apache.org/r/32660/#comment131997>

    this comment can probably be saved.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment131998>

    is this used?



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment131999>

    comment doesn't match



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132007>

    auto isn't whitelisted yet.
    
    here and everywhere.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132013>

    is this c++11?



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132001>

    else use error_message?



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132002>

    use error_message for errors? here and everywhere.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132008>

    should 'net_stats' be in a constant as well?



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132003>

    personally i think just setting them directly is the easist to understand.
    
    see comment below.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132009>

    ditto.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132010>

    s/str/id/



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132000>

    Does it de-duplicate code?
    
    it isn't as easy to read as before to me, only to save the 'if 
(metric.isSome())' part



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132004>

    mind explaining why you call them limiter and fairshare?



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132011>

    also check and output error_message here?



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132005>

    ditto



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32660/#comment132006>

    :)


- Chi Zhang


On April 24, 2015, 7:42 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32660/
> -----------------------------------------------------------
> 
> (Updated April 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: mesos-2332
>     https://issues.apache.org/jira/browse/mesos-2332
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Report network isolator statistics on a per container basis (MESOS-2332)
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 
>   src/slave/containerizer/isolators/network/port_mapping.hpp 
> 466cd82e69665af217d61392b739b9bba16e1e13 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> ccdc44f465f204f674b859c429ba1a6ada51cd88 
>   src/slave/containerizer/mesos/containerizer.hpp 
> fb334e53bc8d73bced402095daddab5e06dc2dd1 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ea3b499258b6be4a643d246159a17a0eaeedf904 
> 
> Diff: https://reviews.apache.org/r/32660/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Reply via email to