> On Nov. 30, 2017, 5:13 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/windows/cpu.hpp > > Lines 66-67 (patched) > > <https://reviews.apache.org/r/63276/diff/4/?file=1905078#file1905078line66> > > > > I'd prefer we define an `Info` struct that has both pid and cpuLimit so > > that we know the lifecycle of these two fields are tied together. THis is > > the conversion we've been following in other isolators. > > ``` > > struct Info' > > { > > Option<pid_t> pid; > > Option<double> cpuLimit; > > }; > > > > hashmap<ContainerID, Info> infos; > > ```
Yeah, this worked out much better! Thanks. Improvments posted here: https://reviews.apache.org/r/64272/ > On Nov. 30, 2017, 5:13 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/windows/cpu.cpp > > Lines 75 (patched) > > <https://reviews.apache.org/r/63276/diff/4/?file=1905079#file1905079line75> > > > > So someone can call `prepare` multiple times if they don't set cpu > > limits? This should be addressed if you use the Info suggestion above. Good catch. This was remedied using the `Info` abstraction. > On Nov. 30, 2017, 5:13 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/windows/cpu.cpp > > Lines 79 (patched) > > <https://reviews.apache.org/r/63276/diff/4/?file=1905079#file1905079line79> > > > > I'd use > > ``` > > const Resources resources = containerConfig.resources(); > > ``` > > > > Using `{...}` is fragile if we add more fields to `Resources` object. I looked it up and couldn't figure out the case where this would become an error, because so long as there isn't a `std::initializer_list` constructor (there isn't, yet, and `T` here isn't an aggregate type as we define ctors for it), then: > All constructors of T participate in overload resolution against the set of > arguments that consists of the elements of the braced-init-list But I fixed it anyway as there must be and edge case I don't understand. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63276/#review192414 ----------------------------------------------------------- On Nov. 30, 2017, 3:34 p.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63276/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2017, 3:34 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. > > These classes are separate derivations of `MesosIsolatorProcess`, > because introducing a `WindowsIsolatorProcess` base class would be > abstraction for the sole purpose of code deduplication. > > Note that these isolators support nesting, and so must support empty > `cpu` or `mem` resources. When these are not provided, the corresponding > code to set the limits is simply not called. > > > Diffs > ----- > > src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056 > src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238 > src/slave/containerizer/mesos/containerizer.cpp > 7f3b86d87cf82429c2627d4a32eb0d5adbcc3f29 > src/slave/containerizer/mesos/isolators/windows.hpp > b0621a5fc411b8812722f7fcf6580ed64dac5382 > src/slave/containerizer/mesos/isolators/windows/cpu.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/windows/cpu.cpp PRE-CREATION > src/slave/containerizer/mesos/isolators/windows/mem.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/windows/mem.cpp PRE-CREATION > src/slave/flags.cpp 7998d9bb66995ad0d1285fcb1042321afdf917a9 > > > Diff: https://reviews.apache.org/r/63276/diff/4/ > > > Testing > ------- > > > Thanks, > > Andrew Schwartzmeyer > >
