----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42618/#review117646 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 171) <https://reviews.apache.org/r/42618/#comment178925> s/netClsHandle/handle/ src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 194) <https://reviews.apache.org/r/42618/#comment178909> s/netClsHandleMgt/handleManager/ `netCls` is already part of the class name, no need to state that in the variable name again. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 407) <https://reviews.apache.org/r/42618/#comment178924> I would just say: "Failed to allocate a net_cls handle for ..." src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 438) <https://reviews.apache.org/r/42618/#comment178929> This is not allocation, right? This is assignment. So ``` // Assign the handle to the cgroup. ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 439) <https://reviews.apache.org/r/42618/#comment178928> No need for this temp variable. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 480) <https://reviews.apache.org/r/42618/#comment178930> Why onAny here? Shouldn't you use .then here? src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 481) <https://reviews.apache.org/r/42618/#comment178913> we align paramters with the first parameter, not '(' ``` return ... .onAny(defer(...), &CgroupsNet... ...); ``` Please do a sweep to fix all such occurances. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 486) <https://reviews.apache.org/r/42618/#comment178914> 2 lines apart. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 488 - 490) <https://reviews.apache.org/r/42618/#comment178915> indentation. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 492) <https://reviews.apache.org/r/42618/#comment178932> s/freeHandle/free/ src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 495) <https://reviews.apache.org/r/42618/#comment178911> Indentation. s/for this cgroup// src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 502) <https://reviews.apache.org/r/42618/#comment178920> `s/_reserveNetClsHandle/recoverHandle/` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 506 - 522) <https://reviews.apache.org/r/42618/#comment178912> Please fix the indentation. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 507) <https://reviews.apache.org/r/42618/#comment178916> s/handle/classid src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 511) <https://reviews.apache.org/r/42618/#comment178919> No need to print hierarchy or cgroup in error message here. It's alwasy callers job to print parameters. That's the protocol we use in the code base. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 514) <https://reviews.apache.org/r/42618/#comment178917> s/netHandle/handle/ src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 518 - 519) <https://reviews.apache.org/r/42618/#comment178918> Error message indentation. - Jie Yu On Feb. 3, 2016, 7:46 a.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42618/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2016, 7:46 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-4345 > https://issues.apache.org/jira/browse/MESOS-4345 > > > Repository: mesos > > > Description > ------- > > Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`. > > > 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/42618/diff/ > > > Testing > ------- > > make and make check > > > Thanks, > > Avinash sridharan > >
