You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2013/04/24 02:43:47 UTC

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.

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

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 ffe637f2f03ff5ca020a4d2cb617be047aade034 

Diff: https://reviews.apache.org/r/10746/diff/


Testing
-------

make check.


Thanks,

Jiang Yan Xu

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On April 24, 2013, 7:34 p.m., Vinod Kone wrote:
> > src/tests/reaper_tests.cpp, lines 181-182
> > <https://reviews.apache.org/r/10746/diff/1/?file=283968#file283968line181>
> >
> >     instead of pulling this into a temp variable, just do processExited.get() (or status.get())

Can't, it causes error: "invalid lvalue in unary ‘&’".


> On April 24, 2013, 7:34 p.m., Vinod Kone wrote:
> > src/tests/reaper_tests.cpp, line 189
> > <https://reviews.apache.org/r/10746/diff/1/?file=283968#file283968line189>
> >
> >     You mean that has exited before that pid was asked to monitor, correct?

Yes. Changed the line to
"Check that the Reaper can monitor a child process that exits before monitor() is called on it."


> On April 24, 2013, 7:34 p.m., Vinod Kone wrote:
> > src/slave/reaper.cpp, lines 125-138
> > <https://reviews.apache.org/r/10746/diff/1/?file=283967#file283967line125>
> >
> >     Considering you always have the permission (because you reject non-permissible ones in monitor) is this necessary?
> >     
> >     Alternatively, is it correct to reject listeners in monitor() if this could still give them result?

Changed the logic so that in monitor() we not only check for alive() (which fails with insufficient permission) but also check (with waitpid) if pid is a child process. When either is satisfied, we add the pid to the map. 
If waitpid actually reaps a terminated pid in monitor() we have to return it directly. 

Then in reap(), all registered pids either 1) satisfy the permission check, or 2) are children of the current process. Therefore we have two code blocks to deal with them. 


- Jiang Yan


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


On April 26, 2013, 7:53 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 26, 2013, 7:53 p.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 ffe637f2f03ff5ca020a4d2cb617be047aade034 
> 
> Diff: https://reviews.apache.org/r/10746/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

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.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19639
-----------------------------------------------------------



src/slave/cgroups_isolator.hpp
<https://reviews.apache.org/r/10746/#comment40549>

    yay!



src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10746/#comment40551>

    i think we were indenting .onAny etc with 2 spaces not 4.
    
    also, each argument should be on a different line.



src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10746/#comment40552>

    formatting.



src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10746/#comment40554>

    What if the future is discarded?



src/slave/process_isolator.cpp
<https://reviews.apache.org/r/10746/#comment40556>

    reorder



src/slave/process_isolator.cpp
<https://reviews.apache.org/r/10746/#comment40558>

    formatting



src/slave/process_isolator.cpp
<https://reviews.apache.org/r/10746/#comment40559>

    formatting



src/slave/process_isolator.cpp
<https://reviews.apache.org/r/10746/#comment40560>

    ditto, check for discarded too



src/slave/reaper.hpp
<https://reviews.apache.org/r/10746/#comment40561>

    Add a comment saying this is a forward declaration.



src/slave/reaper.hpp
<https://reviews.apache.org/r/10746/#comment40562>

    You do send a notification, it's a failed future, correct?
    
    



src/slave/reaper.hpp
<https://reviews.apache.org/r/10746/#comment40563>

    For me 'statuses' really doesn't capture what this map is about. May be 'listeners' or 'receivers'?
    
    Also, why not MultiHashmap?



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment40565>

    To be sure, we only add a pid to 'statuses' map if it has permissions correct? So, we better have permission at this point?



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment40566>

    Can you expand on your comment here?



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment40568>

    Considering you always have the permission (because you reject non-permissible ones in monitor) is this necessary?
    
    Alternatively, is it correct to reject listeners in monitor() if this could still give them result?



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40570>

    s/processExited/status/



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40572>

    Ditto, a comment here that this is parent process.



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40571>

    Can you add a comment here saying this is the child process.



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40573>

    s/processExited/status



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40574>

    instead of pulling this into a temp variable, just do processExited.get() (or status.get())



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40575>

    You mean that has exited before that pid was asked to monitor, correct?



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40576>

    // In child process.



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40577>

    // In parent process



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40578>

    Can you expand the comment on why you do this?



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40579>

    I think you should do an ASSERT on proc::alive(pid)



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40580>

    s/grand/child/  ?



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment40581>

    s/processExited/status


- Vinod Kone


On April 24, 2013, 12:43 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 24, 2013, 12:43 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 ffe637f2f03ff5ca020a4d2cb617be047aade034 
> 
> Diff: https://reviews.apache.org/r/10746/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On April 24, 2013, 8:25 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolator.cpp, line 553
> > <https://reviews.apache.org/r/10746/diff/1/?file=283963#file283963line553>
> >
> >     Can we s/PID<CgroupsIsolator>(this)/self()/ or s/PID<CgroupsIsolator>(this)/this/ now (here and everywhere else)?

Unfortunately neither worked...

Error with self():
../../src/slave/cgroups_isolator.cpp:553: error: no matching function for call to 'defer(process::PID<mesos::internal::slave::Isolator>, void (mesos::internal::slave::CgroupsIsolator::*)(pid_t, const process::Future<Option<int> >&), pid_t&, std::tr1::_Placeholder<1>&)'

Error this this:
../../src/slave/cgroups_isolator.cpp:553: error: no matching function for call to 'defer(mesos::internal::slave::CgroupsIsolator* const, void (mesos::internal::slave::CgroupsIsolator::*)(pid_t, const process::Future<Option<int> >&), pid_t&, std::tr1::_Placeholder<1>&)'


Our guesses were that CgroupsIsolator itself doesn't directly derive from Process<T> but indirectly through "class Isolator : public process::Process<Isolator>".
"PID<CgroupsIsolator>(this)" is still fine, right?


- Jiang Yan


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


On April 24, 2013, 12:43 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 24, 2013, 12:43 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 ffe637f2f03ff5ca020a4d2cb617be047aade034 
> 
> Diff: https://reviews.apache.org/r/10746/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

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.

Posted by Benjamin Hindman <be...@berkeley.edu>.

> On April 24, 2013, 8:25 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolator.cpp, line 553
> > <https://reviews.apache.org/r/10746/diff/1/?file=283963#file283963line553>
> >
> >     Can we s/PID<CgroupsIsolator>(this)/self()/ or s/PID<CgroupsIsolator>(this)/this/ now (here and everywhere else)?
> 
> Jiang Yan Xu wrote:
>     Unfortunately neither worked...
>     
>     Error with self():
>     ../../src/slave/cgroups_isolator.cpp:553: error: no matching function for call to 'defer(process::PID<mesos::internal::slave::Isolator>, void (mesos::internal::slave::CgroupsIsolator::*)(pid_t, const process::Future<Option<int> >&), pid_t&, std::tr1::_Placeholder<1>&)'
>     
>     Error this this:
>     ../../src/slave/cgroups_isolator.cpp:553: error: no matching function for call to 'defer(mesos::internal::slave::CgroupsIsolator* const, void (mesos::internal::slave::CgroupsIsolator::*)(pid_t, const process::Future<Option<int> >&), pid_t&, std::tr1::_Placeholder<1>&)'
>     
>     
>     Our guesses were that CgroupsIsolator itself doesn't directly derive from Process<T> but indirectly through "class Isolator : public process::Process<Isolator>".
>     "PID<CgroupsIsolator>(this)" is still fine, right?

Definitely still fine. I'll need to add an overload of defer that takes a T* and makes sure it's a subclass of ProcessBase. Then you should be able to do 'defer(this, &CgroupsIsolator::reaped, ..., ...)'.


- Benjamin


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


On April 24, 2013, 12:43 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 24, 2013, 12:43 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 ffe637f2f03ff5ca020a4d2cb617be047aade034 
> 
> Diff: https://reviews.apache.org/r/10746/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

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.

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19651
-----------------------------------------------------------



src/slave/cgroups_isolator.hpp
<https://reviews.apache.org/r/10746/#comment40592>

    :-)



src/slave/cgroups_isolator.hpp
<https://reviews.apache.org/r/10746/#comment40593>

    s/processExited/reaped/g



src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10746/#comment40594>

    Can we s/PID<CgroupsIsolator>(this)/self()/ or s/PID<CgroupsIsolator>(this)/this/ now (here and everywhere else)?



src/slave/reaper.hpp
<https://reviews.apache.org/r/10746/#comment40595>

    Just:
    
    // Forward declaration.
    
    Is sufficient.



src/slave/reaper.hpp
<https://reviews.apache.org/r/10746/#comment40596>

    +1 to s/MultiMap/MultiHashmap/.
    
    And I support s/statuses/promises/.


- Benjamin Hindman


On April 24, 2013, 12:43 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 24, 2013, 12:43 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 ffe637f2f03ff5ca020a4d2cb617be047aade034 
> 
> Diff: https://reviews.apache.org/r/10746/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On April 30, 2013, 5:30 p.m., Vinod Kone wrote:
> > src/slave/reaper.cpp, lines 95-96
> > <https://reviews.apache.org/r/10746/diff/4/?file=286029#file286029line95>
> >
> >     does this fit in one line?
> >     
> >     return Future<int>:failed(
> >         "Failed.........................); 
> >     
> >

In this review it still fits in one line. It no longer does when Future<int> gets changed to Future<Option<int> > in https://reviews.apache.org/r/10747
So it is changed to 

    return Future<Option<int> >::failed(
        "Failed to monitor process " + stringify(pid) + ": " + strerror(errno));

there.


- Jiang Yan


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


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

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.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19960
-----------------------------------------------------------

Ship it!



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41176>

    does this fit in one line?
    
    return Future<int>:failed(
        "Failed.........................); 
    
    


- Vinod Kone


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

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On April 30, 2013, 5:35 p.m., Ben Mahler wrote:
> > src/tests/reaper_tests.cpp, line 181
> > <https://reviews.apache.org/r/10746/diff/4/?file=286030#file286030line181>
> >
> >     Don't forget this one! ;)
> >     
> >     s/stat/status_/
> >     
> >     We try to avoid abbreviations.

This is addressed in (a later review) https://reviews.apache.org/r/10747/
Well, because BenH's comment is in that review. I could have changed it here and then I would go there to say "Fixed in https://reviews.apache.org/r/10746/". 
I think the latter is more preferable (fix it when it first appears), right? It'll be that way next time.


- Jiang Yan


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


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

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.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19962
-----------------------------------------------------------



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment41179>

    Don't forget this one! ;)
    
    s/stat/status_/
    
    We try to avoid abbreviations.


- Ben Mahler


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

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On April 30, 2013, 3:15 a.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, lines 70-73
> > <https://reviews.apache.org/r/10746/diff/4/?file=286029#file286029line70>
> >
> >     Ah, this sounds like os::alive(pid_t) is really os::exists(pid_t) or os::valid(pid_t)? I remember discussing renaming os::alive, did you decide to?

The final verdict was to keep the name but give a clear definition in the comment that "A process is not considered alive iff it is terminated AND reaped."


- Jiang Yan


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


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

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.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19934
-----------------------------------------------------------

Ship it!



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41112>

    Ah, this sounds like os::alive(pid_t) is really os::exists(pid_t) or os::valid(pid_t)? I remember discussing renaming os::alive, did you decide to?



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41111>

    Ah, much clearer, thanks!


- Ben Mahler


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

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/
-----------------------------------------------------------

(Updated April 30, 2013, 6:38 p.m.)


Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.


Changes
-------

#include<stout/proc.hpp> -> #include <stout/os.hpp> in reaper_tests.cpp.


Description
-------

See summary.

- Previously the listener was notified when its child processes terminate whether it register them or not. 


Diffs (updated)
-----

  src/slave/cgroups_isolator.hpp 7b8270d0b0f94a71da12bc123d39b44c40c3f7ed 
  src/slave/cgroups_isolator.cpp f12fd48fe739015429a3fe51cc8dce366ec4a483 
  src/slave/process_isolator.hpp 9875f4a6e8e109e31ad390fbd7a84d03ad747190 
  src/slave/process_isolator.cpp 6938fbcca9d384bb014ff4dd52a13763c1f8397a 
  src/slave/reaper.hpp 09844d8d47b143ee369e0c82b19d65a774df4a90 
  src/slave/reaper.cpp b4d8912fa4a5910b0003c06b87a7fbc6164a6382 
  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

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.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19969
-----------------------------------------------------------



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment41183>

    Ah I see thanks!
    
    Yep, if you had fixed it here, the later review would still show the fix after an update (since the parent diffs would have changed).


- Ben Mahler


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

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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.


Changes
-------

Addressed comments.

monitor() return -1 immediately for an invalid pid.


Description
-------

See summary.

- Previously the listener was notified when its child processes terminate whether it register them or not. 


Diffs (updated)
-----

  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

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.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19898
-----------------------------------------------------------



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41042>

    How about you just say "We have permissions to check the validity of the process, so add it to the promises map"?



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41046>

    s/foreach loop/foreach loop but after the while loop/
    
    As discussed offline, mention how the different cases are handled.



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41044>

    s/(alive)// ?


- Vinod Kone


On April 29, 2013, 7:31 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 29, 2013, 7:31 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
> 
>

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.

Posted by Ben Mahler <be...@gmail.com>.

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

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> 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()) {
> >       ...
> >     }

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


> On April 29, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/slave/process_isolator.cpp, lines 465-472
> > <https://reviews.apache.org/r/10746/diff/3/?file=285680#file285680line465>
> >
> >     Ditto

Ditto, see above.


> 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()));

Not using a tmp var causes an error...


> On April 29, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/tests/reaper_tests.cpp, line 99
> > <https://reviews.apache.org/r/10746/diff/3/?file=285683#file285683line99>
> >
> >     Kill this comment, since the spawn'ing happens under the hood now :)

Done.


> On April 29, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/tests/reaper_tests.cpp, line 209
> > <https://reviews.apache.org/r/10746/diff/3/?file=285683#file285683line209>
> >
> >     Kill this comment since the spawn is internal.

Done


> On April 29, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/tests/reaper_tests.cpp, line 215
> > <https://reviews.apache.org/r/10746/diff/3/?file=285683#file285683line215>
> >
> >     newline

Done


> On April 29, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, lines 67-68
> > <https://reviews.apache.org/r/10746/diff/3/?file=285682#file285682line67>
> >
> >     So if the process does not exist, you'll set up the promise here and it will later become ready when the reap() occurs?
> >     
> >     Seems that you want to directly return either:
> >       -An empty Option (-1 in this review), or
> >       -A failure
> >     
> >     Thoughts?

Yes I am doing what your first line says.

We surely can return directly but after we discussed it, probably not a failure because it's then harder to distinguish the two error cases: "process doesn't exist / has exited" and "we don't have permission && process not a child". 

We can return a None() (as in "dictionary get() returns null when the key doesn't exist"). The initial concern was code duplication (logging) but I guess it's better to be more explicit. Either way it probably doesn't hurt performance much.


- Jiang Yan


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


On April 29, 2013, 7:31 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 29, 2013, 7:31 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
> 
>

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On April 29, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, lines 116-119
> > <https://reviews.apache.org/r/10746/diff/3/?file=285682#file285682line116>
> >
> >     Nice comment, what happens when these conditions aren't met?

The method can still handle it. It's hard to write a simple and idempotent CHECK for the precondition so the checkings are embedded in the method body.


- Jiang Yan


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

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.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19881
-----------------------------------------------------------



src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/10746/#comment41009>

    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()) {
      ...
    }



src/slave/process_isolator.cpp
<https://reviews.apache.org/r/10746/#comment41010>

    Ditto



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41012>

    So if the process does not exist, you'll set up the promise here and it will later become ready when the reap() occurs?
    
    Seems that you want to directly return either:
      -An empty Option (-1 in this review), or
      -A failure
    
    Thoughts?



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41013>

    Nice comment, what happens when these conditions aren't met?



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment41014>

    Do you mean if the child terminates before we check if it's alive()? Either way, I think you can kill this comment.



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment41015>

    Kill this comment, since the spawn'ing happens under the hood now :)



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment41027>

    no need to store it in a variable..?
    
    ASSERT_TRUE(WIFSIGNALED(status.get()));
    ASSERT_EQ(SIGKILL, WTERMSIG(status.get()));



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment41029>

    Kill this comment since the spawn is internal.



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment41030>

    newline



src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/10746/#comment41026>

    ditto


- Ben Mahler


On April 29, 2013, 7:31 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 29, 2013, 7:31 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
> 
>

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/
-----------------------------------------------------------

(Updated April 29, 2013, 7:31 a.m.)


Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.


Changes
-------

Addressed comments.


Description
-------

See summary.

- Previously the listener was notified when its child processes terminate whether it register them or not. 


Diffs (updated)
-----

  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

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.

> On April 26, 2013, 9:29 p.m., Vinod Kone wrote:
> > src/slave/reaper.cpp, line 119
> > <https://reviews.apache.org/r/10746/diff/2/?file=285035#file285035line119>
> >
> >     Make a comment on your invariants here.
> >     
> >     Also CHECK them.

Commented on the assumption, no CHECK can be added.


- Jiang Yan


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


On April 29, 2013, 7:31 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 29, 2013, 7:31 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
> 
>

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.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/#review19803
-----------------------------------------------------------


partial review.


src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment40838>

    kill



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment40839>

    expand on the comment that we are failing here because it is neither a child nor do we have perms



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment40840>

    Make a comment on your invariants here.
    
    Also CHECK them.



src/slave/reaper.cpp
<https://reviews.apache.org/r/10746/#comment40841>

    Swap these, so that you can first reap for children and then reap non-children. that way you only need one waitpid()


- Vinod Kone


On April 26, 2013, 7:53 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10746/
> -----------------------------------------------------------
> 
> (Updated April 26, 2013, 7:53 p.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 ffe637f2f03ff5ca020a4d2cb617be047aade034 
> 
> Diff: https://reviews.apache.org/r/10746/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

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.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10746/
-----------------------------------------------------------

(Updated April 26, 2013, 7:53 p.m.)


Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.


Changes
-------

Addressed comments.

Major changes include that 
"Changed the logic so that in monitor() we not only check for alive() (which fails with insufficient permission) but also check (with waitpid) if pid is a child process. When either is satisfied, we add the pid to the map. 
If waitpid actually reaps a terminated pid in monitor() we have to return it directly. 

Then in reap(), all registered pids either 1) satisfy the permission check, or 2) are children of the current process. Therefore we have two code blocks to deal with them. "


Description
-------

See summary.

- Previously the listener was notified when its child processes terminate whether it register them or not. 


Diffs (updated)
-----

  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 ffe637f2f03ff5ca020a4d2cb617be047aade034 

Diff: https://reviews.apache.org/r/10746/diff/


Testing
-------

make check.


Thanks,

Jiang Yan Xu