----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31503/#review74787 -----------------------------------------------------------
src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment121546> Two lines apart between top level entities. src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment121542> The name `Classifier` here is a little confusing because we already have `ip::Classifier`, `icmp::Classifier` and `arc::Classifier`. How about renaming it to `Flow` since the tc command uses `flowid`? src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment121567> The constructors here are a little confusing. For example, what does `min` or `maj` mean. Looks like you just need to set the handle which contains a major and a minor (see code in src/linux/routing/queueing/handle.hpp). src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment121544> One line apart between class methods. src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment121545> Given the class name `Flow`, this method can be renamed to `id` or `handle`. src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment121547> Two lines apart please. src/linux/routing/filter/arp.hpp <https://reviews.apache.org/r/31503/#comment121574> set the specified _flowid_. You need to adjust the comments here. What does that mean for "to a set of links"? :) src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment121600> "Failed to set the flow id: " src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment121598> Add a new line above, please. src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment121603> Please be consistent with other `attach` functions: ``` return Error("Unsupported classifier kind: " + kind); ``` src/tests/routing_tests.cpp <https://reviews.apache.org/r/31503/#comment121606> OK, I think you need to expose the `primiary` and `secondary` in `Handle`. So that here, you can do: ``` action::Flow(Handle(fq_codel::HANDLE.primiary(), 1)); ``` Also, I thought minor=0 is only for the root qdisc? - Jie Yu On Feb. 27, 2015, 9:39 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31503/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2015, 9:39 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 > 410c15ad20ffa694bdb4c4187f1544ef9617ebac > src/linux/routing/filter/arp.hpp fa0ea6f93218fb3bee9f435f0268022d5edc5e11 > src/linux/routing/filter/arp.cpp bf19264ccc83e8a8b8c2affe53779ac070472925 > src/linux/routing/filter/icmp.hpp 7b478c4e23a2fb0895021ce5fb30bdf78131b871 > src/linux/routing/filter/icmp.cpp 86bd67b71a590b88344adbe10fd1b44ea1b5148d > src/linux/routing/filter/internal.hpp > 8a6c0c0390c4ed119a11f0f808e0a244f7734c45 > src/linux/routing/filter/ip.hpp 932ed4bbf57e60261ff3b48ae242a7dd5e1f4260 > src/linux/routing/filter/ip.cpp 922a732c3543a072674208b123fdfadbef2b15f2 > src/tests/routing_tests.cpp 3cda6ab8c1ad24e4b7d0b9aeda2abc595fc857a5 > > Diff: https://reviews.apache.org/r/31503/diff/ > > > Testing > ------- > > Run the testcase. > > > Thanks, > > Cong Wang > >
