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 33040: Expose qdisc statistics from libnl
Date Fri, 10 Apr 2015 00:04:42 GMT

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


This looks good Paul!


src/Makefile.am
<https://reviews.apache.org/r/33040/#comment129080>

    I don't see these files?



src/linux/routing/queueing/fq_codel.hpp
<https://reviews.apache.org/r/33040/#comment129091>

    See my comments below. Move this function to the end (below 'remove').



src/linux/routing/queueing/fq_codel.cpp
<https://reviews.apache.org/r/33040/#comment129092>

    Ditto. Move this function.



src/linux/routing/queueing/ingress.hpp
<https://reviews.apache.org/r/33040/#comment129093>

    Ditto.



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

    Ditto.



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

    Please put this function after `remove` below so that `create` and `remove` are close
to each other (more symmetric).



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

    s/> >/>>/
    
    Here and everywhere else.
    
    Also, we typically do not use const inside hashmap parameter. So please change it to:
    ```
    Try<hashmap<std::string, uint64_t>> statistics(...)
    ```
    
    just to be consistent.



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

    Add an extra blank line above.
    
    Also, s/res/qdisc/



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

    Do you want to return error if qdisc does not exist?



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

    See below. You don't need the temp variable as well.



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

    you only used this temp variable once. We prefer not creating temp variables if unncessary.



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

    s/buf/buffer/
    
    We prefer full names in Mesos.
    
    However, I think we should call it 'name' or 'key' so that the following reads well:
    
    ```
    result[name] = rtnl_tc_get_stat(TC_CAST(qdisc.get().get()), ...);
    ```



src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment129090>

    Not needed anymore?



src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment129088>

    Is this still needed?



src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment129089>

    These are no longer needed?



src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment129095>

    Does this compile? Shouldn't be:
    
    ```
    queueing::ingress::statistics(...);
    queueing::fq_codel::statistics(...);
    ```



src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment129096>

    Insert an extra blank line between these two lines.



src/tests/routing_tests.cpp
<https://reviews.apache.org/r/33040/#comment129079>

    s/ASSERT_TRUE/EXPECT_TRUE/
    
    ASSERT means the test cannot proceed if the result is false. EXPECT means the test can
still proceed.


- Jie Yu


On April 9, 2015, 11:58 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33040/
> -----------------------------------------------------------
> 
> (Updated April 9, 2015, 11:58 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
> -------
> 
> Expose qdisc statistics from libnl
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
>   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/tests/routing_tests.cpp 7cc3b57a3b71544874557d2b1cf88a241b7062ba 
> 
> Diff: https://reviews.apache.org/r/33040/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>


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