You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2013/03/01 01:36:37 UTC
Re: 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/#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>.
> 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
>
>