> On May 4, 2018, 10:53 a.m., Qian Zhang wrote:
> > Thanks for the patch!
> > 
> > The `doXxx()` functions seem a bit strange to me. I am wondering if we can 
> > follow the similar way of how Mesos containerizer calls isolators, e.g., we 
> > could rename `Subsystem` to `SubsystemProcess` and all the `XxxSubsystem` 
> > to `XxxSubsystemProcess` since they are all actually `Process`, and then 
> > introduce a new class `Subsystem` which internally dispatches calls to 
> > `SubsystemProcess`, and `CgroupsIsolatorProcess` always directly calls the 
> > functions of `Subsystem`.
> 
> Benjamin Bannier wrote:
>     Like outlined in the ticket, that would be the other possibility. Note 
> that we not only require dedicated subsystem processes, but also wrappers 
> derived from `Subsystem` for each one (we could use a template here to remove 
> duplicating classes). This could still lead to a lot of duplication in terms 
> of e.g., functions to implement. I got the feeling that the change here was 
> less invasive. Let me know if you think the code is too strange and I can 
> reiterate.
> 
> Gilbert Song wrote:
>     Duplications are tolerable. Let's follow Qian's advice if possible.
> 
> Qian Zhang wrote:
>     > Note that we not only require dedicated subsystem processes, but also 
> wrappers derived from Subsystem for each one.
>     
>     Mind to elaborate a bit on why we need wrappers derived from `Subsystem` 
> class for each one? I think we only need a `Subsystem` class but create 
> multiple objects of it (one for each subsystem), and `Subsystem` should have 
> a member variable `process`, and member functions of `Subsystem` just 
> dispatch calls to the related functions of `process`.

You are right, they are not needed. I updated the patch to use a single wrapper 
class. I also renamed the processes in the preceeding 
https://reviews.apache.org/r/66987/.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202424
-----------------------------------------------------------


On May 7, 2018, 5: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, 5: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
> 
>

Reply via email to