> On Sept. 9, 2014, 4:39 p.m., Niklas Nielsen wrote: > > I think this will work (and good job on style btw!). The biggest piece that > > I find missing is testing; we need tests to verify that the new escalation > > logic works as we expect. > > Tests in case of long (more than 3 deltas) and short (smaller than delta) > > timeouts and combinations of the different levels responding or > > not-responding to SIGTERM and so on. > > Also, we should make sure that there are no surprises when setting the > > grace period. I found it a bit surprising that small timeouts gets chopped > > in halfs and thirds. What happens for 0 second timeouts? > > > > Would it make sense to introduce a helper class that centralize the timeout > > logic/computation? We can add more verification there too and can be used > > to test the logic directly from our unit tests. > > > > Does this make sense?
If the timeout is 0, there will be no graceful shutdown. The chopping policy is similar to that slave uses for hardware resources allocation (see /src/slave/containerizer/containerizer.cpp:105). Having a helper class will definitely help to reuse the code and increase locality, but still each executor should know its own _nested id_ to request the helper for the right timeout. If we want to avoid it, we can put the timeout into the SlaveInfo, and adjust it while propagating the message along the Containerizer->Executor->CommandExecutor chain. > On Sept. 9, 2014, 4:39 p.m., Niklas Nielsen wrote: > > src/launcher/executor.cpp, lines 654-656 > > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line654> > > > > Till raised an issue with namespace aliasing - did you guys sort that > > out? > > I am not a fan either. No, it's still there, raises awarness to this style issue. I haven't found any explicit prohibitions in our style and they are allowed by the Google style, though we don't use them in our code. > On Sept. 9, 2014, 4:39 p.m., Niklas Nielsen wrote: > > src/tests/containerizer.cpp, line 113 > > <https://reviews.apache.org/r/25434/diff/2/?file=683955#file683955line113> > > > > Why 3 seconds? Though default is 5, I decide to set it to 3 (which is the lower minimun for CommandExecutor to give a chance for a task to be reaped). Having it bigger—I suppose—may result in slower tests. > On Sept. 9, 2014, 4:39 p.m., Niklas Nielsen wrote: > > src/slave/constants.hpp, line 54 > > <https://reviews.apache.org/r/25434/diff/2/?file=683947#file683947line54> > > > > Small bit: max columns for comments are 70: > > http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ will fix it. > On Sept. 9, 2014, 4:39 p.m., Niklas Nielsen wrote: > > src/exec/exec.cpp, lines 723-729 > > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line723> > > > > Small nit: We try to keep the variable names short and concise. I would > > have dropped the 'mesos' prefix. Got it. I think we can leave it as it is, since the name length doesn't involve line wrapping. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25434/#review52726 ----------------------------------------------------------- On Sept. 9, 2014, 12:54 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25434/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2014, 12:54 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/slave/flags.hpp 21e0021 > 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 > >
