----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42587/#review117230 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (lines 137 - 139) <https://reviews.apache.org/r/42587/#comment178453> Hum, having two data fields here looks a little confusing to me. How about just merging them: ``` // A map for used handles: primary -> bitset for secondary. hashmap<uint16_t, bitset<0xffff>> used; ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 53) <https://reviews.apache.org/r/42587/#comment178358> Please insert a blank line above. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 53 - 62) <https://reviews.apache.org/r/42587/#comment178433> This is a simple 'contains' check. For readability, I would rather inline this instead of creating a helper method. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 73 - 78) <https://reviews.apache.org/r/42587/#comment178432> I got confused here. minor is a uint16 value. Why this check is necessary? src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 82 - 96) <https://reviews.apache.org/r/42587/#comment178456> For testin, I would probably just expose a 'isUsed' method: ``` bool isUsed(const NetClsHandle& handle); ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 102 - 105) <https://reviews.apache.org/r/42587/#comment178434> See my comments above. I would rather have the 'contains' check here. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 113) <https://reviews.apache.org/r/42587/#comment178455> why minorHandle -1 here? src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 114 - 116) <https://reviews.apache.org/r/42587/#comment178454> Fix error message here (indentation and '+') src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 154) <https://reviews.apache.org/r/42587/#comment178458> I would simply start from position 0 and find the first unset bit. We won't have a lot of containers (O(100) as most). So I won't be too worried about the performance here. Add a TODO to do it more efficiently should be sufficient. - Jie Yu On Jan. 31, 2016, 5:56 p.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42587/ > ----------------------------------------------------------- > > (Updated Jan. 31, 2016, 5:56 p.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-4345 > https://issues.apache.org/jira/browse/MESOS-4345 > > > Repository: mesos > > > Description > ------- > > Implemented the `NetClsHandleMgr` class. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp > b4bc52114389d1c1efce2830f4292bd89bb0de7c > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp > ddc1bf0939e5e8995e6f34fe7b8509b51704f63e > > Diff: https://reviews.apache.org/r/42587/diff/ > > > Testing > ------- > > make > > > Thanks, > > Avinash sridharan > >
