----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/#review84830 -----------------------------------------------------------
Thanks for the efforts! Here are my main sugguestions for this patch: 1) Please split it out. Let's do step by step. 2) Introduce a top level Decipline (similar to Filter). Please let me know if you want to chat about that. src/linux/routing/queueing/ingress.cpp <https://reviews.apache.org/r/34426/#comment136239> Do you have this defined somewhere already? src/linux/routing/queueing/ingress.cpp <https://reviews.apache.org/r/34426/#comment136251> First of all, I think fixing the ingress handle is OK for now. Second, if you really want to change this, you should probably do that in a separate patch. src/linux/routing/queueing/internal.hpp <https://reviews.apache.org/r/34426/#comment136231> These causes unnessary code jumping and it's bad for readability. Could you please revert this change? Thanks! src/linux/routing/queueing/internal.hpp <https://reviews.apache.org/r/34426/#comment136241> I prefer keeping this function simple (just getting all the qdiscs). The filtering can be handled by `getQdisc`. src/linux/routing/queueing/internal.hpp <https://reviews.apache.org/r/34426/#comment136242> As I mentioned above, Let's keep getQdiscs simple and put all the filtering logic in the following for loop. src/linux/routing/queueing/internal.hpp <https://reviews.apache.org/r/34426/#comment136389> Can this be in a separate patch? src/linux/routing/queueing/internal.hpp <https://reviews.apache.org/r/34426/#comment136393> Hum.. Looking at those interfaces make me feel that we should probably introduce a top level Discipline class (not the same as the current one, but similar to Filter): ``` template <typename Config> class Discipline { Handle parent_; Option<Handle> handle_; string kind_; Config config_; }; ``` And the 'create' interface should be: ``` template <typename Config> Try<bool> create( const std::string& _link, const Discipline<Config>& discipline) { ... } ``` Let's try not to return the Handle in this patch! Keep this patch smaller makes it easier for the reviewers! src/linux/routing/queueing/internal.hpp <https://reviews.apache.org/r/34426/#comment136250> ? Please do not sneak in changes like this in an already very huge diff. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/34426/#comment136394> Let's still make the ingress::HANDLE fixed so that we don't need to change port mapping isolator in this patch. - Jie Yu On May 22, 2015, 4:31 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34426/ > ----------------------------------------------------------- > > (Updated May 22, 2015, 4:31 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 > ------- > > Report the network statistics from libnl > > > Diffs > ----- > > 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/slave/containerizer/isolators/network/port_mapping.cpp > 49e983edab598e2ac487bb488fdd12840a9e7dfc > src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 > src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa > > Diff: https://reviews.apache.org/r/34426/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Paul Brett > >
