mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
Date Fri, 22 May 2015 17:58:28 GMT

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


Thanks for the efforts! Here are my main sugguestions for this patch:

1) Please split it out. Let's do step by step.
2) Introduce a top level Decipline (similar to Filter).

Please let me know if you want to chat about that.


src/linux/routing/queueing/ingress.cpp
<https://reviews.apache.org/r/34426/#comment136239>

    Do you have this defined somewhere already?



src/linux/routing/queueing/ingress.cpp
<https://reviews.apache.org/r/34426/#comment136251>

    First of all, I think fixing the ingress handle is OK for now. Second, if you really want
to change this, you should probably do that in a separate patch.



src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34426/#comment136231>

    These causes unnessary code jumping and it's bad for readability. Could you please revert
this change? Thanks!



src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34426/#comment136241>

    I prefer keeping this function simple (just getting all the qdiscs). The filtering can
be handled by `getQdisc`.



src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34426/#comment136242>

    As I mentioned above, Let's keep getQdiscs simple and put all the filtering logic in the
following for loop.



src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34426/#comment136389>

    Can this be in a separate patch?



src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34426/#comment136393>

    Hum.. Looking at those interfaces make me feel that we should probably introduce a top
level Discipline class (not the same as the current one, but similar to Filter):
    
    ```
    template <typename Config>
    class Discipline
    {
      Handle parent_;
      Option<Handle> handle_;
      string kind_;
      Config config_;
    };
    ```
    
    And the 'create' interface should be:
    
    ```
    template <typename Config>
    Try<bool> create(
        const std::string& _link,
        const Discipline<Config>& discipline)
    {
      ...
    }
    ```
    
    Let's try not to return the Handle in this patch! Keep this patch smaller makes it easier
for the reviewers!



src/linux/routing/queueing/internal.hpp
<https://reviews.apache.org/r/34426/#comment136250>

    ? Please do not sneak in changes like this in an already very huge diff.



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

    Let's still make the ingress::HANDLE fixed so that we don't need to change port mapping
isolator in this patch.


- Jie Yu


On May 22, 2015, 4:31 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34426/
> -----------------------------------------------------------
> 
> (Updated May 22, 2015, 4:31 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2665
>     https://issues.apache.org/jira/browse/MESOS-2665
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Report the network statistics from libnl
> 
> 
> Diffs
> -----
> 
>   src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00 
>   src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb 
>   src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b 
>   src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba 
>   src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc

>   src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 
>   src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa 
> 
> Diff: https://reviews.apache.org/r/34426/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>


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