----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45605/#review137040 -----------------------------------------------------------
Sorry about the long wait. I am now volunteer for shepherding this work. Would still love to get this committed. So I re-opened the review. src/linux/routing/queueing/discipline.hpp (line 52) <https://reviews.apache.org/r/45605/#comment202160> This class is already under `namespace queueing {`. So adding another Queueing to class name does not make sense. I would create another file `src/linux/routing/queueing/class.hpp` and move this definition there: ``` namespace queueing { template <typename Config> struct Class {...}; } ``` src/linux/routing/queueing/htb.hpp (line 38) <https://reviews.apache.org/r/45605/#comment202171> Since we not have both `class` and `qdisc`, `exists` become ambiguous. I would rename the exiting funcitons to: ``` existsDiscipline createDiscipline removeDiscipline statisticsDiscipline createClass removeClass ... ``` src/linux/routing/queueing/htb.cpp (lines 41 - 50) <https://reviews.apache.org/r/45605/#comment202172> I think we need two Config now: one for Discipline and one for Class. Looking at https://www.infradead.org/~tgr/libnl/doc/api/group__qdisc__htb.html, the configuration for htb qdisc is different from configuration for htb class. src/linux/routing/queueing/htb.cpp (line 64) <https://reviews.apache.org/r/45605/#comment202175> See my comments below: ``` template <> Try<Nothing> encode<htb::DisciplineConfig>(...); template <> Result<htb::DisciplineConfig> decode<htb::DisciplineConfig>(..); ``` src/linux/routing/queueing/htb.cpp (line 92) <https://reviews.apache.org/r/45605/#comment202174> Given that you have two Config: `DisciplineConfig` and `ClassConfig`, you can still use `encode` here: ``` template <> Try<Nothing> encode<htb::ClassConfig>(...); template <> Result<htb::ClassConfig> decode<htb::ClassConfig>(...); ``` src/linux/routing/queueing/internal.hpp (lines 335 - 350) <https://reviews.apache.org/r/45605/#comment202177> Can you move these two forward declarations up right below `Result<Config> decode(const Netlink<struct rtnl_qdisc>& qdisc);`. Per my comments above, you can rename it to be encode and decode. src/linux/routing/queueing/internal.hpp (lines 356 - 359) <https://reviews.apache.org/r/45605/#comment202178> Can you move this up right below encodeDiscipline. Those are helpers for {en}decoding. - Jie Yu On April 1, 2016, 9:50 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45605/ > ----------------------------------------------------------- > > (Updated April 1, 2016, 9:50 p.m.) > > > Review request for mesos, Ian Downes and Jie Yu. > > > Bugs: mesos-4749 > https://issues.apache.org/jira/browse/mesos-4749 > > > Repository: mesos > > > Description > ------- > > Introduced HTB class API, prepare for per-container bandwidth limit. > > > Diffs > ----- > > src/linux/routing/internal.hpp 8f68119819f7c79ece1a13ac1894b1802ddc8e19 > src/linux/routing/queueing/discipline.hpp > 54d6b214ef6a38fd8279f6d01e6f4e3ccfddf634 > src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 > src/linux/routing/queueing/htb.cpp faadf32bd48cc6bf968b1229789903c0d01fd75c > src/linux/routing/queueing/internal.hpp > 768ed325f9b259e150779eb3ad74f4e5d4bcc7a2 > > Diff: https://reviews.apache.org/r/45605/diff/ > > > Testing > ------- > > make && make check > > > Thanks, > > Cong Wang > >
