mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 25434: Propagate slave shutdown grace period to Executor and CommandExecutor.
Date Fri, 03 Oct 2014 17:25:40 GMT

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


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