You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexander Rukletsov <al...@mesosphere.io> on 2014/10/02 09:17:16 UTC
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/
-----------------------------------------------------------
(Updated Oct. 2, 2014, 7:17 a.m.)
Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
Bugs: MESOS-1571
https://issues.apache.org/jira/browse/MESOS-1571
Repository: mesos-git
Description
-------
The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
Diffs
-----
src/Makefile.am 27c42df
src/exec/exec.cpp e15f834
src/launcher/executor.cpp cbc8750
src/slave/constants.hpp 9030871
src/slave/constants.cpp e1da5c0
src/slave/containerizer/containerizer.hpp 8a66412
src/slave/containerizer/containerizer.cpp 0254679
src/slave/containerizer/docker.cpp 9a29489
src/slave/containerizer/external_containerizer.cpp efbc68f
src/slave/containerizer/mesos/containerizer.cpp 9d08329
src/slave/flags.hpp 32e51d2
src/slave/utils.hpp PRE-CREATION
src/slave/utils.cpp PRE-CREATION
src/tests/containerizer.cpp a17e1e0
src/tests/mesos.hpp 957e223
src/tests/slave_tests.cpp 69be28f
Diff: https://reviews.apache.org/r/25434/diff/
Testing (updated)
-------
make check (OS X 10.9.4)
WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
Thanks,
Alexander Rukletsov
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/exec/exec.cpp, lines 75-76
> > <https://reviews.apache.org/r/25434/diff/4/?file=709150#file709150line75>
> >
> > Why Java executors? I am not sure I understand this comment.
This is what BenH pointed me at. Since Java has garbage collection, an Executor written in java may be garbage collected during the graceful shutdown period. We spawn a separate process exactly to target this problem.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/slave/constants.hpp, lines 51-59
> > <https://reviews.apache.org/r/25434/diff/4/?file=709152#file709152line51>
> >
> > Can we add something similar to the flags help text? The flag is already a bit misleading, so it would be helpful to expand on the new funtionality there.
Agreed, definitely makes sense.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/slave/utils.hpp, lines 29-34
> > <https://reviews.apache.org/r/25434/diff/4/?file=709160#file709160line29>
> >
> > Can you include an example? How about the sequence charts from the JIRA? The logic of adjustShutdownTimeout is still not crystal clear to me.
> > To put it in another way; it is hard for me to tell whether we cover all corner cases.
The approach I took gives two guarantees: a timeout is always nonnegative and not greater than the parent one. We can get a situation when the timeout at one or more levels is ridiculously small, which effectively disables the graceful shutdown. Do you have any specific concerns?
I have added the sequence chart and expanded the comment a bit.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/slave/utils.hpp, line 35
> > <https://reviews.apache.org/r/25434/diff/4/?file=709160#file709160line35>
> >
> > How about 'getShutdownTimeout'? It is not really adjusting, but rather partition the total timeout into sub-timeouts - correct?
> >
> > Also, we can hide the implementation adjustShutdownTimeout in utils.cpp so we only export adjustExecutorShutdownTimeout and adjustCommandExecutorShutdownTimeout (which could be called 'getExecutorShutdownTimeout'?)
'getShutdownTimeout' implies the timeout is not calculated on the fly based on input parameters. 'getShutdownTimeoutForLevel' is better, but too long. How about 'calculateShutdownTimeout' for the basic function and 'get...' for the specific ones? I would also leave the basic function publicly accessible for potential use in other places and levels.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/slave/utils.cpp, line 42
> > <https://reviews.apache.org/r/25434/diff/4/?file=709161#file709161line42>
> >
> > Do we want to have checks/asserts to ensure we have positive/non-zero timeouts?
I think currently it's difficult to supply a negative duration, but agree, let's do it for the future.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/tests/mesos.hpp, lines 865-868
> > <https://reviews.apache.org/r/25434/diff/4/?file=709163#file709163line865>
> >
> > You only use this in MesosExecutorForceShutdown - how about moving it to slave_tests.cpp instead?
> >
> > How about using 'statusMatchesTask' or 'statusHasTaskId'. 'relatedTo' seems a bit informal/imprecise.
Changed name to `statusMatchesTask ()`. We do not have utility functions in `slave_tests.cpp`, and the predicate is rather generic, so I would leave it in the `tests/mesos.hpp` where it can be found faster by the next one who needs the same functionality.
I remove this test from this request and move it to the newly created [JIRA ticket](https://issues.apache.org/jira/browse/MESOS-1871).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55348
-----------------------------------------------------------
On Oct. 2, 2014, 7:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
>
>
> Bugs: MESOS-1571
> https://issues.apache.org/jira/browse/MESOS-1571
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
>
>
> Diffs
> -----
>
> src/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> src/slave/constants.hpp 9030871
> src/slave/constants.cpp e1da5c0
> src/slave/containerizer/containerizer.hpp 8a66412
> src/slave/containerizer/containerizer.cpp 0254679
> src/slave/containerizer/docker.cpp 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55348
-----------------------------------------------------------
Hi Alex,
Sorry for the tardy reply on this review. I have left a few high-level comments - think that Tim touched upon most of the style issues.
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment95718>
Why Java executors? I am not sure I understand this comment.
src/slave/constants.hpp
<https://reviews.apache.org/r/25434/#comment95726>
Can we add something similar to the flags help text? The flag is already a bit misleading, so it would be helpful to expand on the new funtionality there.
src/slave/utils.hpp
<https://reviews.apache.org/r/25434/#comment95715>
Can you include an example? How about the sequence charts from the JIRA? The logic of adjustShutdownTimeout is still not crystal clear to me.
To put it in another way; it is hard for me to tell whether we cover all corner cases.
src/slave/utils.hpp
<https://reviews.apache.org/r/25434/#comment95714>
How about 'getShutdownTimeout'? It is not really adjusting, but rather partition the total timeout into sub-timeouts - correct?
Also, we can hide the implementation adjustShutdownTimeout in utils.cpp so we only export adjustExecutorShutdownTimeout and adjustCommandExecutorShutdownTimeout (which could be called 'getExecutorShutdownTimeout'?)
src/slave/utils.cpp
<https://reviews.apache.org/r/25434/#comment95716>
Do we want to have checks/asserts to ensure we have positive/non-zero timeouts?
src/tests/mesos.hpp
<https://reviews.apache.org/r/25434/#comment95717>
You only use this in MesosExecutorForceShutdown - how about moving it to slave_tests.cpp instead?
How about using 'statusMatchesTask' or 'statusHasTaskId'. 'relatedTo' seems a bit informal/imprecise.
- Niklas Nielsen
On Oct. 2, 2014, 12:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 12:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
>
>
> Bugs: MESOS-1571
> https://issues.apache.org/jira/browse/MESOS-1571
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
>
>
> Diffs
> -----
>
> src/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> src/slave/constants.hpp 9030871
> src/slave/constants.cpp e1da5c0
> src/slave/containerizer/containerizer.hpp 8a66412
> src/slave/containerizer/containerizer.cpp 0254679
> src/slave/containerizer/docker.cpp 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 2, 2014, 12:49 a.m., Timothy Chen wrote:
> > src/slave/utils.cpp, line 39
> > <https://reviews.apache.org/r/25434/diff/4/?file=709161#file709161line39>
> >
> > Log the timeout value
Both the one that is too small and the effective one.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55196
-----------------------------------------------------------
On Oct. 2, 2014, 12:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 12:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
>
>
> Bugs: MESOS-1571
> https://issues.apache.org/jira/browse/MESOS-1571
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
>
>
> Diffs
> -----
>
> src/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> src/slave/constants.hpp 9030871
> src/slave/constants.cpp e1da5c0
> src/slave/containerizer/containerizer.hpp 8a66412
> src/slave/containerizer/containerizer.cpp 0254679
> src/slave/containerizer/docker.cpp 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Oct. 2, 2014, 7:49 a.m., Timothy Chen wrote:
> > src/exec/exec.cpp, line 739
> > <https://reviews.apache.org/r/25434/diff/4/?file=709150#file709150line739>
> >
> > I'm not sure this should be at the WARNING level, as it's not really expected to have it set all the time. IMHO it should be INFO
We actually expect it to be set explicitly in order to avoid situations where slave uses one value and executor another.
> On Oct. 2, 2014, 7:49 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 1023
> > <https://reviews.apache.org/r/25434/diff/4/?file=709164#file709164line1023>
> >
> > Times(1) is default, can ignore the call.
To the best of my knowledge, we always explicitly write `Times(1)` in such cases.
> On Oct. 2, 2014, 7:49 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 1061
> > <https://reviews.apache.org/r/25434/diff/4/?file=709164#file709164line1061>
> >
> > EXPECT_TRUE
Good catch!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55196
-----------------------------------------------------------
On Oct. 2, 2014, 7:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
>
>
> Bugs: MESOS-1571
> https://issues.apache.org/jira/browse/MESOS-1571
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
>
>
> Diffs
> -----
>
> src/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> src/slave/constants.hpp 9030871
> src/slave/constants.cpp e1da5c0
> src/slave/containerizer/containerizer.hpp 8a66412
> src/slave/containerizer/containerizer.cpp 0254679
> src/slave/containerizer/docker.cpp 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 2, 2014, 7:49 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 1023
> > <https://reviews.apache.org/r/25434/diff/4/?file=709164#file709164line1023>
> >
> > Times(1) is default, can ignore the call.
>
> Alexander Rukletsov wrote:
> To the best of my knowledge, we always explicitly write `Times(1)` in such cases.
:) Well I got commented on about it by other committers, so I'll let you decide what to do here...
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55196
-----------------------------------------------------------
On Oct. 7, 2014, 3:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 3:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
>
>
> Bugs: MESOS-1571
> https://issues.apache.org/jira/browse/MESOS-1571
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
>
>
> Diffs
> -----
>
> src/Makefile.am cff38ce
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> src/slave/constants.hpp 9030871
> src/slave/constants.cpp e1da5c0
> src/slave/containerizer/containerizer.hpp 8a66412
> src/slave/containerizer/containerizer.cpp 0254679
> src/slave/containerizer/docker.cpp 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 16f0cc2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.4, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55196
-----------------------------------------------------------
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment95590>
I'm not sure this should be at the WARNING level, as it's not really expected to have it set all the time. IMHO it should be INFO
src/slave/utils.cpp
<https://reviews.apache.org/r/25434/#comment95591>
I think the usual style is to move mesos::internal::slave::SHUTDOWN_TIMEOUT_DELTA *
to the next line
src/slave/utils.cpp
<https://reviews.apache.org/r/25434/#comment95592>
Log the timeout value
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/25434/#comment95593>
Times(1) is default, can ignore the call.
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/25434/#comment95594>
EXPECT_TRUE
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/25434/#comment95595>
ditto
- Timothy Chen
On Oct. 2, 2014, 7:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
>
>
> Bugs: MESOS-1571
> https://issues.apache.org/jira/browse/MESOS-1571
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
>
>
> Diffs
> -----
>
> src/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> src/slave/constants.hpp 9030871
> src/slave/constants.cpp e1da5c0
> src/slave/containerizer/containerizer.hpp 8a66412
> src/slave/containerizer/containerizer.cpp 0254679
> src/slave/containerizer/docker.cpp 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55662
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25434]
All tests passed.
- Mesos ReviewBot
On Oct. 7, 2014, 3:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 3:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
>
>
> Bugs: MESOS-1571
> https://issues.apache.org/jira/browse/MESOS-1571
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
>
>
> Diffs
> -----
>
> src/Makefile.am cff38ce
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> src/slave/constants.hpp 9030871
> src/slave/constants.cpp e1da5c0
> src/slave/containerizer/containerizer.hpp 8a66412
> src/slave/containerizer/containerizer.cpp 0254679
> src/slave/containerizer/docker.cpp 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 16f0cc2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.4, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/
-----------------------------------------------------------
(Updated Oct. 7, 2014, 3:54 p.m.)
Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
Changes
-------
Address issues.
Bugs: MESOS-1571
https://issues.apache.org/jira/browse/MESOS-1571
Repository: mesos-git
Description
-------
The configurable slave's executor_shutdown_grace_period flag is propagated to Executor and CommandExecutor through an environment variable. Shutdown timeout in Executor and signal escalation timeout in CommandExecutor are now dependent on this flag. Each nested timeout is somewhat shorter than the parent one.
Diffs (updated)
-----
src/Makefile.am cff38ce
src/exec/exec.cpp e15f834
src/launcher/executor.cpp cbc8750
src/slave/constants.hpp 9030871
src/slave/constants.cpp e1da5c0
src/slave/containerizer/containerizer.hpp 8a66412
src/slave/containerizer/containerizer.cpp 0254679
src/slave/containerizer/docker.cpp 9a29489
src/slave/containerizer/external_containerizer.cpp efbc68f
src/slave/containerizer/mesos/containerizer.cpp 9d08329
src/slave/flags.hpp 16f0cc2
src/slave/utils.hpp PRE-CREATION
src/slave/utils.cpp PRE-CREATION
src/tests/containerizer.cpp a17e1e0
src/tests/slave_tests.cpp 69be28f
Diff: https://reviews.apache.org/r/25434/diff/
Testing (updated)
-------
make check (Mac OS 10.9.4, Ubuntu 14.04)
Thanks,
Alexander Rukletsov