----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25434/#review52689 -----------------------------------------------------------
Ship it! This really is some awesome work Alex. Well drafted within the JIRA, well implemented while keeping the amount of configuration variables to the existing minimum. Some smaller nitpicking below ;) ... src/exec/exec.cpp <https://reviews.apache.org/r/25434/#comment91652> This should be const, no? src/exec/exec.cpp <https://reviews.apache.org/r/25434/#comment91654> This could be a const reference. Same is true for the above, I guess. src/exec/exec.cpp <https://reviews.apache.org/r/25434/#comment91682> Even though you do perfectly explain things within constants.hpp, I would love to see a tiny bit more information on the origin of the delta multipliers. src/launcher/executor.cpp <https://reviews.apache.org/r/25434/#comment91655> We never used namespace aliasing within mesos, hence I am entirely unsure if it is ok to do so. On the other hand, I feel using it like you did in this case is perfectly valid and improves the readability of the sources. src/launcher/executor.cpp <https://reviews.apache.org/r/25434/#comment91681> Even though you do perfectly explain things within constants.hpp, I would love to see a tiny bit more information on the origin of the delta multipliers. src/slave/constants.hpp <https://reviews.apache.org/r/25434/#comment91658> I really like this approach. Just wanted to make sure we do not create timeout's (using the /2 or /3 arithmetics) that are unrealistic for the underlaying polling mechanisms. Wouldnt we get hard shutdown in 100% of all attempts once the user supplies a period below 3 seconds (due to the fact that the reaper has a period of 1 second, hence at least 1 second should be waited before triggering a SIGKILL). - Till Toenshoff On Sept. 8, 2014, 8:04 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25434/ > ----------------------------------------------------------- > > (Updated Sept. 8, 2014, 8:04 p.m.) > > > Review request for mesos, 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/exec/exec.cpp 36d1778 > src/launcher/executor.cpp 12ac14b > 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 0febbac > src/slave/containerizer/external_containerizer.cpp efbc68f > src/slave/containerizer/mesos/containerizer.cpp 9d08329 > src/tests/containerizer.cpp a17e1e0 > > Diff: https://reviews.apache.org/r/25434/diff/ > > > Testing > ------- > > make check (OS X 10.9.4; Ubuntu 14.04 amd64) > > > Thanks, > > Alexander Rukletsov > >
