----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31503/#review75386 -----------------------------------------------------------
src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment122402> Add a space before the colon. Also, how about calling it `action::Flow` instead of `action::FlowId`. It reads better since we have an `id()` member method: ``` Try<bool> create( ... const action::Flow& flow) { xxx = flow.id(); } ``` src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment122409> Rename `_handle` to `_id`? src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment122423> Why we need this constructor? IMO, it's not needed. We can always do the following at the callsites, right? ``` Flow(parent.primary(), 0x1) ``` src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment122408> Rename it to id_? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment122410> Adjust the comments accordingly (using action 'flow' consistently). What's `Classid` in your comment? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment122412> `Failed to set u32 classid: "...` src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment122411> In the comments above, you said you only care about u32 filters. Why you set the target here? Maybe you should adjust your comments above. - Jie Yu On March 4, 2015, 8:06 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31503/ > ----------------------------------------------------------- > > (Updated March 4, 2015, 8:06 p.m.) > > > Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu. > > > Bugs: MESOS-2422 > https://issues.apache.org/jira/browse/MESOS-2422 > > > Repository: mesos > > > Description > ------- > > Introduce a new action which saves the classid for a specified filter. > Similar to the u32 terminal action, it is not a real action defined by TC, it > is just for setting a filter attribute. The classid or flowid is used to > match the fq_codel flows. > > > Diffs > ----- > > src/linux/routing/filter/action.hpp 410c15a > src/linux/routing/filter/arp.hpp fa0ea6f > src/linux/routing/filter/arp.cpp bf19264 > src/linux/routing/filter/icmp.hpp 7b478c4 > src/linux/routing/filter/icmp.cpp 86bd67b > src/linux/routing/filter/internal.hpp 8a6c0c0 > src/linux/routing/filter/ip.hpp 932ed4b > src/linux/routing/filter/ip.cpp 922a732 > src/linux/routing/queueing/handle.hpp 2725d07 > src/tests/routing_tests.cpp 3cda6ab > > Diff: https://reviews.apache.org/r/31503/diff/ > > > Testing > ------- > > Run the testcase. > > > Thanks, > > Cong Wang > >
