mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request: Replaced some names in cgroups and updated some return values to be Try<bool> instead of Try<Nothing>.
Date Mon, 26 Nov 2012 18:46:04 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message