> On Nov. 15, 2012, 10:11 p.m., Ben Mahler wrote: > > src/linux/cgroups.hpp, line 193 > > <https://reviews.apache.org/r/8058/diff/1/?file=189976#file189976line193> > > > > So this one is called 'kill' but it's for sending arbitrary signals to > > all processes in a cgroup? Do we enforce the signal is of type SIGKILL, > > SIGQUIT, etc.?
This is called 'kill' to provide the syscall 'kill' functionality for cgroups (which already exists for processes and process groups). Since this is for sending an arbitrary signal, it does not enforce only SIGKILL or SIGQUIT. This primitive is useful for implementing a few of our primitives such as 'destroy', but it also seemed generally useful, and thus worth providing. Note that I eliminated 'killTasks' because it shares a lot in common with 'destroy' (and I felt having two very similar things would be more confusing). Instead, I factored out the signal sending part of 'killTasks' into 'kill'. My thought was that we can "extend" destroy in the future to perform non-recursive destroys if we want to get some of the 'killTasks' functionality back (which isn't needed outside of the cgroups.cpp). > On Nov. 15, 2012, 10:11 p.m., Ben Mahler wrote: > > src/linux/cgroups.cpp, line 626 > > <https://reviews.apache.org/r/8058/diff/1/?file=189977#file189977line626> > > > > Hm.. this is kind of a backwards way to do error handling compared to > > how we do it everywhere else. > > > > Why the switch from Try<Nothing> to Option<string>? First, this is different than 'checkHierarchy' which has been replaced with 'mounted'. Second, this is an internal function that is being used to verify a hierarchy (possibly a cgroup, and possibly a control) and return a formatted error string if an error has occurred. (I suppose the comment above 'verify' was insufficient, but this function is effectively called "verifyOrReturnFormattedErrorString".) I choose not to return Try<Nothing> because I wanted the semantics that the returned error string was "complete", and I wouldn't need to add any contextual information at the call site. This is not the case (or should not be the case) for other things that return Try<*> (e.g., mkdir). In fact, this review includes numerous changes where I'm adding missing contextual information. Said another way, 99.9% of all cases where we "propagate" an error from a Try<*> we add some contextual information, and when that contextual information is not there I (we) tend try and determine whether or not that is okay or we need to add something (again, I did this a bunch in this review). I didn't want to have to remember why I didn't include contextual information for a Try<Nothing> returned from 'verify', so instead I choose to make the returned error string more explicit. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8058/#review13483 ----------------------------------------------------------- On Nov. 14, 2012, 11:57 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8058/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2012, 11:57 p.m.) > > > Review request for mesos, Vinod Kone and Ben Mahler. > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/linux/cgroups.hpp 1351134dc8b40fb7d7d692c4dd24f0efa24eadb0 > src/linux/cgroups.cpp 039e2b4121edc419e9f1a48a7c0b99ca77aa44ff > src/slave/cgroups_isolation_module.cpp > 8211618d7729350654e2d17946c5b912ed9dda6a > src/tests/assert.hpp 62fde12fa7ddddbbc9e8812f26314af5307a02df > src/tests/cgroups_tests.cpp 85e81854a6767bad5c19f6411130d154c2870d99 > > Diff: https://reviews.apache.org/r/8058/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
