> On May 8, 2018, 5 p.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp > > Line 174 (original), 236 (patched) > > <https://reviews.apache.org/r/66635/diff/3/?file=2017556#file2017556line236> > > > > 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()`?
I went this way because we do not really have a clear pattern on how to safely mark wrapped functions which are safe to access concurrently. I think it makes sense to expose the function directly (as opposed to accessing it through a `dispatch`) since the function itself is `const`, so I removed `subsystemName`. > On May 8, 2018, 5 p.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp > > Lines 122 (patched) > > <https://reviews.apache.org/r/66635/diff/3/?file=2017557#file2017557line122> > > > > Mind to explain why we need `std::move` here? Want to leverage move > > constructor? Semantically an `Owned` cannot be copied since it models unique ownership. MESOS-5122 should address making it impossible to copy instances, but until it is fixed we need to use `Owned` carefully and manually `move` instead of copying it implictly. > On May 8, 2018, 5 p.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp > > Lines 136 (patched) > > <https://reviews.apache.org/r/66635/diff/3/?file=2017557#file2017557line136> > > > > 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? What would the benefit of that be? It would require manual memory management I believe we should avoid since we have a library (`Owned`) doing it for us. Please feel free to reopen if I am missing something. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66635/#review202619 ----------------------------------------------------------- On May 8, 2018, 5:33 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66635/ > ----------------------------------------------------------- > > (Updated May 8, 2018, 5:33 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/4/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
