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 34321: Merge class Handle which is duplicated between filter/handle and queueing/handle.
Date Thu, 21 May 2015 22:09:03 GMT

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

Ship it!


Looks good to me. A few style nits. Thanks for the merging! This is great!


src/linux/routing/filter/basic.hpp
<https://reviews.apache.org/r/34321/#comment136173>

    Move this above `#include "linux/routing/filter/action.hpp"` and insert a blank line between
them.



src/linux/routing/filter/basic.cpp
<https://reviews.apache.org/r/34321/#comment136172>

    Move this right above `#include "linux/routing/internal.hpp"`



src/linux/routing/filter/filter.hpp
<https://reviews.apache.org/r/34321/#comment136171>

    Move this above `#include "linux/routing/filter/action.hpp"` and insert a blank line between
them.



src/linux/routing/filter/handle.hpp
<https://reviews.apache.org/r/34321/#comment136170>

    Swap these two. We include C headers first.



src/linux/routing/filter/handle.hpp
<https://reviews.apache.org/r/34321/#comment136166>

    I don't think you need the `routing::` prefix here.



src/linux/routing/filter/icmp.hpp
<https://reviews.apache.org/r/34321/#comment136169>

    Move this above `#include "linux/routing/filter/action.hpp"` and insert a blank line between
them.



src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/34321/#comment136168>

    Move this right above `#include "linux/routing/internal.hpp"`



src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/34321/#comment136167>

    Move this right above `#include "linux/routing/internal.hpp"`



src/linux/routing/filter/ip.hpp
<https://reviews.apache.org/r/34321/#comment136174>

    DItto.



src/linux/routing/filter/ip.cpp
<https://reviews.apache.org/r/34321/#comment136175>

    Ditto.



src/linux/routing/handle.hpp
<https://reviews.apache.org/r/34321/#comment136160>

    Please swap these two and add a space between them. <stdint.h> is considered a C
header and <ostream> is a C++ header.



src/linux/routing/handle.hpp
<https://reviews.apache.org/r/34321/#comment136165>

    Please add a blank line between methods.



src/linux/routing/handle.hpp
<https://reviews.apache.org/r/34321/#comment136161>

    I would use one line for each of those functions:
    
    ```
    uint16_t primary() const { return handle >> 16; }
    uint16_t secondary() const { return handle & 0x0000ffff; }
    ...
    ```



src/linux/routing/handle.hpp
<https://reviews.apache.org/r/34321/#comment136162>

    Please put this funciton outside the Handle class. See src/linux/routing/filter/ip.hpp.
    
    BTW, is this funciton used anywhere? We don't usually introduce a function that's not
needed.



src/linux/routing/handle.hpp
<https://reviews.apache.org/r/34321/#comment136164>

    I don't think this is a good practice and we don't have to follow what `tc` command does.
What not always print the secondary?



src/linux/routing/queueing/handle.hpp
<https://reviews.apache.org/r/34321/#comment136176>

    +1 on Cong's comment. Please adjust the comments accordingly.



src/tests/routing_tests.cpp
<https://reviews.apache.org/r/34321/#comment136177>

    Move this right above `#include "linux/routing/route.hpp"`


- Jie Yu


On May 19, 2015, 8:27 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34321/
> -----------------------------------------------------------
> 
> (Updated May 19, 2015, 8:27 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
> -------
> 
> Merge class Handle which is duplicated between filter/handle and queueing/handle.
> 
> 
> Diffs
> -----
> 
>   src/linux/routing/filter/basic.hpp 99b0b058633403e334e7230dfa7d705c99b3cb10 
>   src/linux/routing/filter/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c 
>   src/linux/routing/filter/filter.hpp 024582cf8274da2e5bcccc4ebd00fcf83d6930e2 
>   src/linux/routing/filter/handle.hpp 190177441bc95cf77690dbdeca189a816c6e2324 
>   src/linux/routing/filter/icmp.hpp d55eeee8f1e31036e75780c52690e812de265ec6 
>   src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 
>   src/linux/routing/filter/internal.hpp c74098dab97807084e6630998da354265680c763 
>   src/linux/routing/filter/ip.hpp 62bb5f89d35fc9622fca303aa01a789fcfbf2f76 
>   src/linux/routing/filter/ip.cpp 0d25e2d4f0ac4c3f151ad40e796dbdbc54dba9fa 
>   src/linux/routing/handle.hpp PRE-CREATION 
>   src/linux/routing/queueing/handle.hpp 5f0cb7775f9190caba6b85cabf9019a97b2a7de2 
>   src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa 
> 
> Diff: https://reviews.apache.org/r/34321/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>


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