----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42618/#review117916 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 38) <https://reviews.apache.org/r/42618/#comment179186> No need for this. src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 160) <https://reviews.apache.org/r/42618/#comment179180> kill this line src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 161) <https://reviews.apache.org/r/42618/#comment179202> Hum, this should be Option<NetClsHandle> handle. src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 167) <https://reviews.apache.org/r/42618/#comment179182> Can you use an empty IntervetSet to represent the None case? I.e., no need to use Option here. src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 174) <https://reviews.apache.org/r/42618/#comment179178> No need for the leading underscore. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 200) <https://reviews.apache.org/r/42618/#comment179184> What's defaultPrimary? Who is the user? Can you just do the following: ``` if (!primaries.empty()) { handleManager = NetClsHandleManager(primaries.get()); } ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 243 - 245) <https://reviews.apache.org/r/42618/#comment179187> No need to for this. Can you set the default behavior to be not allocating handles? You may need to set a default value for 'primaries' in CgroupsNetClsIsolatorProcess constructor. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 265 - 267) <https://reviews.apache.org/r/42618/#comment179188> Make it less jagged. Also, no need to quote containerId because it's generated by Mesos (guaranteed no space in it, you only need to quote something that user provides). ``` return Failure( "Failed to check cgroup for container " + stringify(containerId)); ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 284 - 285) <https://reviews.apache.org/r/42618/#comment179189> Ditto on remove quote for containerId. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 320 - 322) <https://reviews.apache.org/r/42618/#comment179190> Ditto. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 407 - 408) <https://reviews.apache.org/r/42618/#comment179191> This fits one line? src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 408) <https://reviews.apache.org/r/42618/#comment179193> Can you make 'defaultPrimiary' a helper method instead of a field member. The field member will go away in the future once we introduce other allocation policies like one primary per framweork. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 412) <https://reviews.apache.org/r/42618/#comment179197> space after ':' s/for the container// src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 441) <https://reviews.apache.org/r/42618/#comment179198> Ditto on removing quote. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 449) <https://reviews.apache.org/r/42618/#comment179200> I would rather use handle.isSome as the condition here. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 503) <https://reviews.apache.org/r/42618/#comment179201> I would use handle.isSOme() as the condition here. - Jie Yu On Feb. 4, 2016, 1:42 a.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42618/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2016, 1:42 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 > >
