> 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.? > > Benjamin Hindman wrote: > 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).
I missed that this matches the kill syscall, sounds good. > 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>? > > Benjamin Hindman wrote: > 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. Alright, I'd prefer to keep the consistency, but I see you put a lot of thought into this. - Ben ----------------------------------------------------------- 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 > >
