You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2012/12/13 11:18:25 UTC
Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs
-----
src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
src/linux/proc.hpp 6c84a73ab1c4d6893390f96eae15422c21001328
src/linux/proc.cpp 99df77447ddfb07a5febec9ebfed396778665041
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/slave_recovery_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
> On Dec. 17, 2012, 10:22 p.m., Benjamin Hindman wrote:
> > src/linux/proc.cpp, line 235
> > <https://reviews.apache.org/r/8570/diff/1/?file=238067#file238067line235>
> >
> > Yes, this way we can do testing on OS X.
done. moved to stout/proc.hpp
> On Dec. 17, 2012, 10:22 p.m., Benjamin Hindman wrote:
> > src/tests/slave_recovery_tests.cpp, line 109
> > <https://reviews.apache.org/r/8570/diff/1/?file=238071#file238071line109>
> >
> > You should be able to do 'listener.self()' rather than save the 'mpl' variable.
good to know.
> On Dec. 17, 2012, 10:22 p.m., Benjamin Hindman wrote:
> > src/slave/reaper.cpp, line 93
> > <https://reviews.apache.org/r/8570/diff/1/?file=238069#file238069line93>
> >
> > Ahh, why aren't we checking for an error?
logged a warning.
> On Dec. 17, 2012, 10:22 p.m., Benjamin Hindman wrote:
> > src/slave/reaper.cpp, line 95
> > <https://reviews.apache.org/r/8570/diff/1/?file=238069#file238069line95>
> >
> > Won't this always send a notify even if the pid doesn't exist?
Yes, but this is inside if (!alive.get()), this process terminated.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review14619
-----------------------------------------------------------
On Dec. 19, 2012, 1:34 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp 6c84a73ab1c4d6893390f96eae15422c21001328
> src/linux/proc.cpp 99df77447ddfb07a5febec9ebfed396778665041
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review14619
-----------------------------------------------------------
src/linux/proc.cpp
<https://reviews.apache.org/r/8570/#comment31033>
Yes, this way we can do testing on OS X.
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment31035>
No need to have status declared above, just declare it in this block and also declare it below (down by the other waitpid call). That way you don't need to "reset" the status.
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment31036>
Ahh, why aren't we checking for an error?
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment31037>
Won't this always send a notify even if the pid doesn't exist?
src/tests/slave_recovery_tests.cpp
<https://reviews.apache.org/r/8570/#comment31034>
You should be able to do 'listener.self()' rather than save the 'mpl' variable.
- Benjamin Hindman
On Dec. 13, 2012, 10:18 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2012, 10:18 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp 6c84a73ab1c4d6893390f96eae15422c21001328
> src/linux/proc.cpp 99df77447ddfb07a5febec9ebfed396778665041
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/slave_recovery_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.hpp, line 47
> > <https://reviews.apache.org/r/8570/diff/3/?file=249260#file249260line47>
> >
> > Are you able to programmatically enforce that one of these holds?
>
> Vinod Kone wrote:
> not easily, no. esp. 1 and 2, in posix compliant way.
>
> more importantly, what do you mean by "enforce"? what do you propose we do when one of those 3 conditions aren't satisfied? log a warning?
Well what is the consequence of none of these being satisfied, and consequently, the notification not being sent?
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.hpp, line 51
> > <https://reviews.apache.org/r/8570/diff/3/?file=249260#file249260line51>
> >
> > I think this is more succinct without losing clarity:
> > s/addProcess/monitor
>
> Vinod Kone wrote:
> I like addProcess because of its similarity to addListener. But, I could be convinced otherwise.
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, line 96
> > <https://reviews.apache.org/r/8570/diff/3/?file=249261#file249261line96>
> >
> > But what about condition 3 you listed, where we are a privileged user?
>
> Vinod Kone wrote:
> Then we souldn't enter this if loop.
Can you add a comment as to why?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review15415
-----------------------------------------------------------
On Jan. 24, 2013, 12:17 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 12:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am 0ab59b75b2955c532d0833f132bdaffe323838d0
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
> src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
> src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 11f8b36d19313f65f1b191a781c6d5ed90130099
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > Looks like you forgot to add stout/proc.hpp?
gah. sorry. done
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.hpp, line 47
> > <https://reviews.apache.org/r/8570/diff/3/?file=249260#file249260line47>
> >
> > Are you able to programmatically enforce that one of these holds?
not easily, no. esp. 1 and 2, in posix compliant way.
more importantly, what do you mean by "enforce"? what do you propose we do when one of those 3 conditions aren't satisfied? log a warning?
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.hpp, line 51
> > <https://reviews.apache.org/r/8570/diff/3/?file=249260#file249260line51>
> >
> > I think this is more succinct without losing clarity:
> > s/addProcess/monitor
I like addProcess because of its similarity to addListener. But, I could be convinced otherwise.
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.hpp, line 61
> > <https://reviews.apache.org/r/8570/diff/3/?file=249260#file249260line61>
> >
> > Why do we use a set for the listeners?
> > Can you please change this to a list?
> >
> > std::set is ordered, and our listeners do not provide comparison operations
good catch. done
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, line 96
> > <https://reviews.apache.org/r/8570/diff/3/?file=249261#file249261line96>
> >
> > But what about condition 3 you listed, where we are a privileged user?
Then we souldn't enter this if loop.
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/tests/reaper_tests.cpp, line 68
> > <https://reviews.apache.org/r/8570/diff/3/?file=249263#file249263line68>
> >
> > Don't use PLOG within a child! We may deadlock, right?
fair enough. using EXIT instead.
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/tests/reaper_tests.cpp, line 76
> > <https://reviews.apache.org/r/8570/diff/3/?file=249263#file249263line76>
> >
> > kill newline
I like to have a new line between 2 comment blocks if the first one is more general and applies to more than the 2nd block. makes sense?
> On Jan. 16, 2013, 11:07 p.m., Ben Mahler wrote:
> > src/tests/reaper_tests.cpp, line 80
> > <https://reviews.apache.org/r/8570/diff/3/?file=249263#file249263line80>
> >
> > ditto, let's not use any glog stuff in child processes
using EXIT
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review15415
-----------------------------------------------------------
On Jan. 16, 2013, 9:54 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2013, 9:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp 6c84a73ab1c4d6893390f96eae15422c21001328
> src/linux/proc.cpp 99df77447ddfb07a5febec9ebfed396778665041
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 6b68c69b07b831255d34a68d7b9b4b37eee55167
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review15415
-----------------------------------------------------------
Looks like you forgot to add stout/proc.hpp?
src/linux/proc.cpp
<https://reviews.apache.org/r/8570/#comment33250>
newline
src/linux/proc.cpp
<https://reviews.apache.org/r/8570/#comment33251>
shouldn't this be ordered before sys?
src/slave/reaper.hpp
<https://reviews.apache.org/r/8570/#comment33259>
Not yours, but seems overly verbose:
s/addProcessExitedListener/addListener
src/slave/reaper.hpp
<https://reviews.apache.org/r/8570/#comment33256>
Are you able to programmatically enforce that one of these holds?
src/slave/reaper.hpp
<https://reviews.apache.org/r/8570/#comment33257>
I think this is more succinct without losing clarity:
s/addProcess/monitor
src/slave/reaper.hpp
<https://reviews.apache.org/r/8570/#comment33260>
Why do we use a set for the listeners?
Can you please change this to a list?
std::set is ordered, and our listeners do not provide comparison operations
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment33253>
ditto on ordered before sys?
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment33264>
Use "Failed ..." format?
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment33265>
But what about condition 3 you listed, where we are a privileged user?
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33267>
bad indentation
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33273>
Don't use PLOG within a child! We may deadlock, right?
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33269>
formatting
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33270>
kill newline
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33274>
ditto, let's not use any glog stuff in child processes
- Ben Mahler
On Jan. 16, 2013, 9:54 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2013, 9:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp 6c84a73ab1c4d6893390f96eae15422c21001328
> src/linux/proc.cpp 99df77447ddfb07a5febec9ebfed396778665041
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 6b68c69b07b831255d34a68d7b9b4b37eee55167
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 14, 2013, 12:57 a.m., Ben Mahler wrote:
> > src/slave/reaper.hpp, line 49
> > <https://reviews.apache.org/r/8570/diff/5/?file=251415#file251415line49>
> >
> > I think these primitives are more clear as:
> >
> > listen(Listener)
> > monitor(pid_t)
> >
> > The external api user shouldn't be thinking about "adding" a pid, it's more that they want the reaper to "monitor" the pid.
> >
> > Likewise with listen, although the verb applies more to the listener itself: Listener.listen(reaper); Because it's not the reaper doing the listening, it's the listener doing the listening. So maybe it's best to leave this one as addListener for now.
> >
> > Certainly once this returns a Future, listen makes sense!
> >
> > reaper.listen(pid_t).onAny(...);
s/addProcess/monitor/
> On Feb. 14, 2013, 12:57 a.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, line 78
> > <https://reviews.apache.org/r/8570/diff/5/?file=251416#file251416line78>
> >
> > what's going on with the scoping here..?
> >
> > this loop pid is hiding the above pid?
will bring it down to where its actually used.
> On Feb. 14, 2013, 12:57 a.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, line 99
> > <https://reviews.apache.org/r/8570/diff/5/?file=251416#file251416line99>
> >
> > Here, you're passing a potentially uninitialized status to the listener!
> >
> > Instead this should notify the listeners with the fact that the status is unknown.
> >
> > Maybe the Future refactoring needs to happen now since it's difficult for you to propagate the error back out to the listeners?
There is no documentation on what 'status' is going to be when waitpid returns an error. Fwiw, in the example given in the man page for waitpid, they do not initialize the status variable.
I'm going to initialize it to -1, just in case.
Reg: futures instead of callbacks, I'm not going to do this as part of this review. It is more involved and needs some thinking through.
> On Feb. 14, 2013, 12:57 a.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, line 106
> > <https://reviews.apache.org/r/8570/diff/5/?file=251416#file251416line106>
> >
> > can you move the pid declaration down to here and do the if check separately from the assignment?
> >
> > Also, shouldn't this be a loop here, until this returns -1, I imagine we'd want to pick up all the exited children here rather than only process 1 per second? i.e. changing this if to a while should be sufficient, right?
s/if/while
> On Feb. 14, 2013, 12:57 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/proc.hpp, line 35
> > <https://reviews.apache.org/r/8570/diff/5/?file=251425#file251425line35>
> >
> > Wait.. why is this file called proc.hpp?
> >
> > That makes me think of linux/proc.hpp inside mesos, which operates on the linux-only proc pseudo-filesystem.
> >
> > I think for now, we can come up with a different namespace than proc, or you could just stuff this in os.hpp, what do you think?
I could name this either proc.hpp or process.hpp. I don't think having same filenames is that big of a concern, as long as they are in different namespaces. We already do this (e.g. we have 3 different state.hpp files).
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review16540
-----------------------------------------------------------
On Jan. 24, 2013, 12:17 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 12:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am 0ab59b75b2955c532d0833f132bdaffe323838d0
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
> src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
> src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 11f8b36d19313f65f1b191a781c6d5ed90130099
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review16540
-----------------------------------------------------------
src/slave/reaper.hpp
<https://reviews.apache.org/r/8570/#comment35105>
I like this TODO!
src/slave/reaper.hpp
<https://reviews.apache.org/r/8570/#comment35096>
I think these primitives are more clear as:
listen(Listener)
monitor(pid_t)
The external api user shouldn't be thinking about "adding" a pid, it's more that they want the reaper to "monitor" the pid.
Likewise with listen, although the verb applies more to the listener itself: Listener.listen(reaper); Because it's not the reaper doing the listening, it's the listener doing the listening. So maybe it's best to leave this one as addListener for now.
Certainly once this returns a Future, listen makes sense!
reaper.listen(pid_t).onAny(...);
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment35106>
what's going on with the scoping here..?
this loop pid is hiding the above pid?
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment35103>
Here, you're passing a potentially uninitialized status to the listener!
Instead this should notify the listeners with the fact that the status is unknown.
Maybe the Future refactoring needs to happen now since it's difficult for you to propagate the error back out to the listeners?
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment35107>
can you move the pid declaration down to here and do the if check separately from the assignment?
Also, shouldn't this be a loop here, until this returns -1, I imagine we'd want to pick up all the exited children here rather than only process 1 per second? i.e. changing this if to a while should be sufficient, right?
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment35109>
s/In parent process/Still in child process.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment35110>
s/(grand) child/grandchild
third_party/libprocess/include/stout/proc.hpp
<https://reviews.apache.org/r/8570/#comment35104>
Wait.. why is this file called proc.hpp?
That makes me think of linux/proc.hpp inside mesos, which operates on the linux-only proc pseudo-filesystem.
I think for now, we can come up with a different namespace than proc, or you could just stuff this in os.hpp, what do you think?
- Ben Mahler
On Jan. 24, 2013, 12:17 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 12:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am 0ab59b75b2955c532d0833f132bdaffe323838d0
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
> src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
> src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 11f8b36d19313f65f1b191a781c6d5ed90130099
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review16904
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Feb. 20, 2013, 1:46 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2013, 1:46 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
> src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
> On March 1, 2013, 12:36 a.m., Ben Mahler wrote:
> > src/slave/reaper.hpp, line 56
> > <https://reviews.apache.org/r/8570/diff/8/?file=261399#file261399line56>
> >
> > Would be great to have monitor return an error when one of these three conditions is not met, no? Seems like not doing this will cause some tricky bugs!
done. monitor returns Try<Nothing>.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review17214
-----------------------------------------------------------
On Feb. 23, 2013, 8:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2013, 8:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
> src/sched/sched.cpp 25a78e88ac84c340533b87187afb0984d1c0401b
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
> src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
> src/tests/master_tests.cpp e140f409bf6e651cc365b5fb8d064ceae1cd8ed8
> src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 468df4e1426b725cf1252300134bc13658eae090
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review17214
-----------------------------------------------------------
Ship it!
src/slave/reaper.hpp
<https://reviews.apache.org/r/8570/#comment36514>
Would be great to have monitor return an error when one of these three conditions is not met, no? Seems like not doing this will cause some tricky bugs!
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment36516>
can you add on strerror(errno)?
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment36515>
Please at least add a TODO to make status an Option, if you don't want to take that on now. Currently, we're passing a special value around (-1).
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment36517>
Why do you log here but not above? If you're going to log, can you do so in both places? Also, can you include the status?
third_party/libprocess/include/stout/proc.hpp
<https://reviews.apache.org/r/8570/#comment36519>
s/If we are here, we/We/
Actually can you please kill this comment? Seems to be only referring to EPERM and not EINVAL. Better to not comment on all error scenarios and let strerror(errno) speak for itself.
- Ben Mahler
On Feb. 23, 2013, 8:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2013, 8:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
> src/sched/sched.cpp 25a78e88ac84c340533b87187afb0984d1c0401b
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
> src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
> src/tests/master_tests.cpp e140f409bf6e651cc365b5fb8d064ceae1cd8ed8
> src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 468df4e1426b725cf1252300134bc13658eae090
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated March 13, 2013, 6:12 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased off trunk for posterity. no need for review.
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
src/sched/sched.cpp 25a78e88ac84c340533b87187afb0984d1c0401b
src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
src/slave/lxc_isolation_module.cpp 1ed3c87f884089e032bba2ed89be109abe6c1c13
src/slave/process_based_isolation_module.cpp ff98d105af675dfc66070feaa43b42c1aa438fd8
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
src/tests/cgroups_tests.cpp 65c2bfaec1dd5adc879a206aa97592ae7f0c0074
src/tests/master_tests.cpp 2ba14fcaade0580970196f0eb8dbcd5ce573797e
src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
third_party/libprocess/third_party/stout/Makefile.am 4d5f50c0e34d99dc45a042e9baff6f70be1c5719
third_party/libprocess/third_party/stout/include/stout/proc.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated March 2, 2013, 12:52 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's.
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am 851237cbf8db071d27de40c01d702514713b861a
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
src/sched/sched.cpp 25a78e88ac84c340533b87187afb0984d1c0401b
src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
src/slave/lxc_isolation_module.cpp 1ed3c87f884089e032bba2ed89be109abe6c1c13
src/slave/process_based_isolation_module.cpp ff98d105af675dfc66070feaa43b42c1aa438fd8
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
src/tests/cgroups_tests.cpp f9be02bc84b26892799cf28d037d13b4bb71df30
src/tests/master_tests.cpp 104610cef3fdaaea75271657c42c4fd2b6b61618
src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
src/tests/utils.hpp b648b631d8c11c26e6adfa3f5b16012044557e25
third_party/libprocess/third_party/stout/Makefile.am 15e09d3e9e5e722cc88393132f40448485cd48ab
third_party/libprocess/third_party/stout/include/stout/proc.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated Feb. 23, 2013, 8:12 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp da30e9a860ba7a142015baf05feaa508bdcdfed5
src/sched/sched.cpp 25a78e88ac84c340533b87187afb0984d1c0401b
src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
src/tests/master_tests.cpp e140f409bf6e651cc365b5fb8d064ceae1cd8ed8
src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
third_party/libprocess/Makefile.am 468df4e1426b725cf1252300134bc13658eae090
third_party/libprocess/include/stout/proc.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated Feb. 20, 2013, 1:46 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
third_party/libprocess/include/stout/proc.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 15, 2013, 2:02 a.m., Ben Mahler wrote:
> > src/slave/reaper.cpp, line 103
> > <https://reviews.apache.org/r/8570/diff/6/?file=258674#file258674line103>
> >
> > You're passing -1 to the listeners here! Can you propagate an error instead when the call fails?
> >
> > Right now you log the warning, but then notify anyway. This should either not notify them, or notify them of the error, I lack the context to know which is more appropriate atm.
We definitely should notify them, because the process exited. We send -1 status, when we can't determine status. The slave appropriately logs "unknown signal" when it gets this executorTerminated().
> On Feb. 15, 2013, 2:02 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/proc.hpp, line 33
> > <https://reviews.apache.org/r/8570/diff/6/?file=258684#file258684line33>
> >
> > What else do you see going in here?
> >
> > I'd imagine linux/proc.hpp moving to stout, at which point do you expect to consolidate the two? If not, then we may want to move this function into os.hpp.
I can imagine things like checking if a process is orphaned, sending signals (w/ appropriate error checking) etc to go here. Basically any helpers that make it easy to deal with processes.
linux/proc.hpp is linux specific. So, when we move it into stout, I would imagine it be under linux directory in stout.
I don't like to include it in os.hpp because 1) its already bloated and 2) proc would be a useful abstraction to deserve its own file.
> On Feb. 15, 2013, 2:02 a.m., Ben Mahler wrote:
> > src/tests/process_spawn.cpp, line 28
> > <https://reviews.apache.org/r/8570/diff/6/?file=258679#file258679line28>
> >
> > This file is outside our coding conventions, can you add a TODO to clean it up?
cleaned up.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review16624
-----------------------------------------------------------
On Feb. 14, 2013, 3:30 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2013, 3:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
> src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review16624
-----------------------------------------------------------
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment35288>
You're passing -1 to the listeners here! Can you propagate an error instead when the call fails?
Right now you log the warning, but then notify anyway. This should either not notify them, or notify them of the error, I lack the context to know which is more appropriate atm.
src/tests/process_spawn.cpp
<https://reviews.apache.org/r/8570/#comment35290>
This file is outside our coding conventions, can you add a TODO to clean it up?
third_party/libprocess/include/stout/proc.hpp
<https://reviews.apache.org/r/8570/#comment35289>
What else do you see going in here?
I'd imagine linux/proc.hpp moving to stout, at which point do you expect to consolidate the two? If not, then we may want to move this function into os.hpp.
- Ben Mahler
On Feb. 14, 2013, 3:30 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2013, 3:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
> src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated Feb. 14, 2013, 3:30 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's and rebased off trunk.
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
third_party/libprocess/include/stout/proc.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated Jan. 24, 2013, 12:17 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
BenH's comments.
Rebased off trunk
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am 0ab59b75b2955c532d0833f132bdaffe323838d0
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/slave/solaris_project_isolation_module.cpp f3b6a68926af34c46873d8de1c9858480f42ef98
src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
third_party/libprocess/Makefile.am 11f8b36d19313f65f1b191a781c6d5ed90130099
third_party/libprocess/include/stout/proc.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 22, 2013, 4:21 a.m., Benjamin Hindman wrote:
> > src/tests/reaper_tests.cpp, lines 45-47
> > <https://reviews.apache.org/r/8570/diff/4/?file=250446#file250446line45>
> >
> > In other tests, we used 'ASSERT_NE(-1, pipe(pipes));'. While I like the idea of getting more debug info from the PLOG, I also like the idea of only aborting this test case. Does a PASSERT exist?
agreed on aborting only this test case. couldn't find anything akin to PASSERT.
> On Jan. 22, 2013, 4:21 a.m., Benjamin Hindman wrote:
> > src/tests/reaper_tests.cpp, line 54
> > <https://reviews.apache.org/r/8570/diff/4/?file=250446#file250446line54>
> >
> > I'm guilty of this too, but we really should not treat 'pid' as a boolean, because it is not. We should probably do a general "sweep" through the code and rectify everywhere.
done. here and everywhere else.
> On Jan. 22, 2013, 4:21 a.m., Benjamin Hindman wrote:
> > src/tests/reaper_tests.cpp, line 70
> > <https://reviews.apache.org/r/8570/diff/4/?file=250446#file250446line70>
> >
> > This is a better place for doing something like PLOG, but you'll want to use perror since a child process might have the dreaded glog fork deadlock. Note that you _DO_ want to kill the entire process in this case otherwise you'll have two processes executing test cases, which gets very confusing for the person running tests.
agreed. fixed
> On Jan. 22, 2013, 4:21 a.m., Benjamin Hindman wrote:
> > src/tests/reaper_tests.cpp, lines 79-80
> > <https://reviews.apache.org/r/8570/diff/4/?file=250446#file250446line79>
> >
> > If you want to be sure, use a pipe.
I decided to be lazy and use a while loop instead.
> On Jan. 22, 2013, 4:21 a.m., Benjamin Hindman wrote:
> > src/slave/reaper.cpp, line 47
> > <https://reviews.apache.org/r/8570/diff/4/?file=250444#file250444line47>
> >
> > Cool. I like the cleanup, but I still think the long term solution here is to return a Future that gets satisfied once a pid is gone (reaped by us or otherwise).
as discussed offline, I'm going to make this a TODO for now.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review15556
-----------------------------------------------------------
On Jan. 19, 2013, 1:52 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2013, 1:52 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am 0ab59b75b2955c532d0833f132bdaffe323838d0
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 11f8b36d19313f65f1b191a781c6d5ed90130099
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/#review15556
-----------------------------------------------------------
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment33584>
Cool. I like the cleanup, but I still think the long term solution here is to return a Future that gets satisfied once a pid is gone (reaped by us or otherwise).
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment33574>
Move to just above line 93 please.
src/slave/reaper.cpp
<https://reviews.apache.org/r/8570/#comment33573>
Swap these two lines.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33575>
In other tests, we used 'ASSERT_NE(-1, pipe(pipes));'. While I like the idea of getting more debug info from the PLOG, I also like the idea of only aborting this test case. Does a PASSERT exist?
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33576>
I'm guilty of this too, but we really should not treat 'pid' as a boolean, because it is not. We should probably do a general "sweep" through the code and rectify everywhere.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33579>
Ditto above.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33577>
This is a better place for doing something like PLOG, but you'll want to use perror since a child process might have the dreaded glog fork deadlock. Note that you _DO_ want to kill the entire process in this case otherwise you'll have two processes executing test cases, which gets very confusing for the person running tests.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33580>
If you want to be sure, use a pipe.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33582>
Ditto EXIT comment above.
src/tests/reaper_tests.cpp
<https://reviews.apache.org/r/8570/#comment33583>
s/while(1/while (true/
- Benjamin Hindman
On Jan. 19, 2013, 1:52 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8570/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2013, 1:52 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
>
>
> Diffs
> -----
>
> src/Makefile.am 0ab59b75b2955c532d0833f132bdaffe323838d0
> src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
> src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/reaper_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
> third_party/libprocess/Makefile.am 11f8b36d19313f65f1b191a781c6d5ed90130099
> third_party/libprocess/include/stout/proc.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8570/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated Jan. 19, 2013, 1:52 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's comments and rebased off trunk
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am 0ab59b75b2955c532d0833f132bdaffe323838d0
src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c
src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
third_party/libprocess/Makefile.am 11f8b36d19313f65f1b191a781c6d5ed90130099
third_party/libprocess/include/stout/proc.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated Jan. 16, 2013, 9:54 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's comments
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
src/linux/proc.hpp 6c84a73ab1c4d6893390f96eae15422c21001328
src/linux/proc.cpp 99df77447ddfb07a5febec9ebfed396778665041
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
third_party/libprocess/Makefile.am 6b68c69b07b831255d34a68d7b9b4b37eee55167
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Five): Implemented non-child process
monitoring in reaper
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8570/
-----------------------------------------------------------
(Updated Dec. 19, 2012, 1:34 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
moved reaper tests to its own test file.
DOESNT address BenH's comments yet.
Description
-------
Needed this to properly monitor the exit status of re-connected executors, as they will be parented by INIT.
Diffs (updated)
-----
src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
src/linux/proc.hpp 6c84a73ab1c4d6893390f96eae15422c21001328
src/linux/proc.cpp 99df77447ddfb07a5febec9ebfed396778665041
src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1
src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/reaper_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8570/diff/
Testing
-------
make check
Thanks,
Vinod Kone