> On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/windows.hpp > > Lines 47 (patched) > > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line48> > > > > Can you move the implementation to a cpp file (instead of putting in > > the header)?
Gladly! > On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/windows.hpp > > Lines 74-82 (patched) > > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line75> > > > > That's the default implementation in MesosIsolatorProcess. Simply > > remove this method? Good catch. > On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/windows.hpp > > Lines 35-41 (original), 100-109 (patched) > > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line101> > > > > Remove this given this is the default behavor in parent class? Good catch. > On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/windows.hpp > > Line 60 (original), 153 (patched) > > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line159> > > > > I think we should call `update` in `isolate` too. Otherwise, the > > process won't be subject to cpu/mem limitation until the next `update` is > > called. > > > > I you think inheritence is hard, maybe just get rid the base class. a > > little bit code duplication is not too bad and will make it much easier to > > read. I'm not sure I totally get you. Joe had told me (that is, if I remember correctly), that both `isolate` and `update` will be called before the task is considered "started" the first time. I guess we should verify this, and if it isn't the case, then `isolate` should also call the same code as `update`. As for: > I you think inheritence is hard, maybe just get rid the base class. a little > bit code duplication is not too bad and will make it much easier to read. What design were you suggesting? Currently the `WindowsMemIsolatorProcess` and `WindowsCpuIsolatorProcess` classes inherit from `WindowsIsolatorProcess`, which inherits from `MesosIsolatorProcess`. Are you saying that removing the intermediate class, `WindowsIsolatorProcess`, would look cleaner? > On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/windows.hpp > > Line 64 (original), 210 (patched) > > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line216> > > > > 2 lines apart. Gotcha. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63276/#review190505 ----------------------------------------------------------- On Nov. 6, 2017, 3:18 p.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63276/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2017, 3:18 p.m.) > > > Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John > Kordich, Joseph Wu, Li Li, and Michael Park. > > > Bugs: MESOS-5462 and MESOS-6690 > https://issues.apache.org/jira/browse/MESOS-5462 > https://issues.apache.org/jira/browse/MESOS-6690 > > > Repository: mesos > > > Description > ------- > > Instead of deriving from the POSIX isolators, we now have two real > Windows isolators that can be used together or separately. The `Cpu` > isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a > hard-cap memory limit on the job object for the container. > > > Diffs > ----- > > src/slave/containerizer/mesos/containerizer.cpp > 100e3bbda543d87808da9ff6bea42da5099ea8c5 > src/slave/containerizer/mesos/isolators/windows.hpp > b0621a5fc411b8812722f7fcf6580ed64dac5382 > src/slave/flags.cpp 63aaac218fdc067742d39f1fc8497723784d9595 > > > Diff: https://reviews.apache.org/r/63276/diff/2/ > > > Testing > ------- > > > Thanks, > > Andrew Schwartzmeyer > >
