----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66635/#review202619 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp Line 174 (original), 236 (patched) <https://reviews.apache.org/r/66635/#comment284559> It seems `subsystemName` is only used in `Subsystem::name()`, so instead of introducing this member variable, can we just call `process->name()` in `Subsystem::name()`? src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp Lines 121 (patched) <https://reviews.apache.org/r/66635/#comment284525> No need `process::`? src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp Lines 122 (patched) <https://reviews.apache.org/r/66635/#comment284536> Mind to explain why we need `std::move` here? Want to leverage move constructor? src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp Lines 136 (patched) <https://reviews.apache.org/r/66635/#comment284565> What about just making this method's return type as `Try<Subsystem*>` and then `return new Subsystem(subsystemProcess.get())` at the end of this method? src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp Line 83 (original), 173 (patched) <https://reviews.apache.org/r/66635/#comment284524> No need `process::`? Ditto for all other places in this file. src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp Line 92 (original), 175 (patched) <https://reviews.apache.org/r/66635/#comment284523> No need `std::`? Ditto for all other places in this file. src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp Line 108 (original), 199 (patched) <https://reviews.apache.org/r/66635/#comment284522> These two parameters should be in two lines. - Qian Zhang On May 7, 2018, 11:56 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66635/ > ----------------------------------------------------------- > > (Updated May 7, 2018, 11:56 p.m.) > > > Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu. > > > Bugs: MESOS-8786 > https://issues.apache.org/jira/browse/MESOS-8786 > > > Repository: mesos > > > Description > ------- > > Different cgroups subsystems are modelled as actors. In this patch we > introduce wrapper classes which `dispatch` to the processes. This > removes e.g., races from mixing naked and `dispatch`'ed method calls. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp > 5763c9880728f02e44116fd50e62b592a8ef69b6 > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp > 4431ce13d28035b0c5c037b2848ae03aeaf65562 > src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp > 65c1e47a569f320b63b54e5f4fc1da374d02ee0d > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp > 9afa02b207e6272836e5a36d69fb48f1f4d02150 > > > Diff: https://reviews.apache.org/r/66635/diff/3/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
