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: Changed reaper to use Future as the notification mechanism. The notification is now sent only if the pid is explicitly registered via the monitor() call.
Date Tue, 30 Apr 2013 03:15:54 GMT


> On April 29, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/tests/reaper_tests.cpp, lines 184-186
> > <https://reviews.apache.org/r/10746/diff/3/?file=285683#file285683line184>
> >
> >     no need to store it in a variable..?
> >     
> >     ASSERT_TRUE(WIFSIGNALED(status.get()));
> >     ASSERT_EQ(SIGKILL, WTERMSIG(status.get()));
> 
> Jiang Yan Xu wrote:
>     Not using a tmp var causes an error...

Ok, status_ would be better then (we try not to use abbreviations for variable names).


> On April 29, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolator.cpp, lines 806-813
> > <https://reviews.apache.org/r/10746/diff/3/?file=285678#file285678line806>
> >
> >     Can you add a little more context to the logging?
> >     
> >     "Failed to monitor " << pid << ": " << status.isDiscarded()
? "discarded" : status.failure();
> >     
> >     With the ternary you can catch both cases:
> >     if (!status.ready()) {
> >       ...
> >     }
> 
> Jiang Yan Xu wrote:
>     This is again unfortunately done in a later commit. Sometimes the review on these
lines came at a later review just got corrected in that review... I does make things untidy
I'll make bigger effort on this in the future.
>     
>     Fixed at: https://reviews.apache.org/r/10747

Much appreciated!!


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19881
-----------------------------------------------------------


On April 30, 2013, 1:21 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 30, 2013, 1:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
> 
> 
> Description
> -------
> 
> See summary.
> 
> - Previously the listener was notified when its child processes terminate whether it
register them or not. 
> 
> 
> Diffs
> -----
> 
>   src/slave/cgroups_isolator.hpp f8fabc4e1c3c303b35a76db96b4b2479bd7c8ff8 
>   src/slave/cgroups_isolator.cpp 8b79da50d8fb0c2c8716dd7d2c734b65c32f60b4 
>   src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190 
>   src/slave/process_isolator.cpp 6e2af87d291d7c3448393c1ffa816f7020e2dff6 
>   src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90 
>   src/slave/reaper.cpp bd3dcef07c370ad338b478755bf8f7ce6408e4a3 
>   src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3 
>   src/tests/utils.hpp ca3ecd7f0cab283327bf83e57d4b405b4ada9c74 
> 
> Diff: https://reviews.apache.org/r/10746/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


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