> On Nov. 20, 2014, 6:26 p.m., Niklas Nielsen wrote: > > src/Makefile.am, line 292 > > <https://reviews.apache.org/r/28065/diff/2/?file=765853#file765853line292> > > > > s/grace/graceful/? If so, then do a scan over the files.
I think it makes sense to be consistent. We agreed for "grace_shutdown" for the proto message. However, I'm not a native speaker. I remember BenM used "grace shutdown" in replies on the dev list. > On Nov. 20, 2014, 6:26 p.m., Niklas Nielsen wrote: > > src/slave/constants.hpp, lines 51-63 > > <https://reviews.apache.org/r/28065/diff/2/?file=765854#file765854line51> > > > > Sweet comment! > > > > The timeout isn't for the task, right? Only for the executor - so we > > need to remove '[Task]' from the nest diagram, no? Well, we can neither control or enforce the shutdown on the task level. However, the executor may use this value and propagate it to its tasks. I tend to leave it as it is to address the most general case, but if you still find it confusing, please re-open the issue and I'll correct. > On Nov. 20, 2014, 6:26 p.m., Niklas Nielsen wrote: > > src/slave/constants.hpp, lines 60-61 > > <https://reviews.apache.org/r/28065/diff/2/?file=765854#file765854line60> > > > > We need to think in general terms; the command executor is just one we > > provide. You're right, thanks. > On Nov. 20, 2014, 6:26 p.m., Niklas Nielsen wrote: > > src/slave/grace_shutdown.hpp, line 37 > > <https://reviews.apache.org/r/28065/diff/2/?file=765856#file765856line37> > > > > /<-- shutdown_grace_period/<-- shutdown_grace_period./ It's `flags.shutdown_grace_period` wrapped and indented by 2 spaces. Or that is not your point? > On Nov. 20, 2014, 6:26 p.m., Niklas Nielsen wrote: > > src/slave/grace_shutdown.cpp, line 46 > > <https://reviews.apache.org/r/28065/diff/2/?file=765857#file765857line46> > > > > Do you need 'mesos::internal::slave' when you are already in the > > namespace? > > > > Here and below (we may be able to get rid of some of the wrappings) Good catch! > On Nov. 20, 2014, 6:26 p.m., Niklas Nielsen wrote: > > src/slave/grace_shutdown.cpp, line 63 > > <https://reviews.apache.org/r/28065/diff/2/?file=765857#file765857line63> > > > > Executor vs CommandExecutor is confusing - do you mean ExecutorProcess > > vs Executor? I gave the functions more general names for clarity. > On Nov. 20, 2014, 6:26 p.m., Niklas Nielsen wrote: > > src/slave/grace_shutdown.cpp, line 71 > > <https://reviews.apache.org/r/28065/diff/2/?file=765857#file765857line71> > > > > This is a noop right? Why not return baseShutdownTimeout immediately? That was done for consistency, so that the claculation logic is in `calculateGracePeriod()`, while other functions define only levels. Do you think we should optimize and return `baseShutdownTimeout` directly? - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28065/#review62291 ----------------------------------------------------------- On Nov. 17, 2014, 12:10 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28065/ > ----------------------------------------------------------- > > (Updated Nov. 17, 2014, 12:10 p.m.) > > > Review request for mesos, Ben Mahler, Niklas Nielsen, and Till Toenshoff. > > > Bugs: MESOS-1571 > https://issues.apache.org/jira/browse/MESOS-1571 > > > Repository: mesos-git > > > Description > ------- > > Shutdown grace period differs across shutdown levels (containerizer, > ExecutorProcess, CommandExecutor). Each nested timeout is somewhat shorter > than the parent one. > > > Diffs > ----- > > src/Makefile.am 0fe7dd0 > src/slave/constants.hpp fd1c1ab > src/slave/constants.cpp 2a99b11 > src/slave/grace_shutdown.hpp PRE-CREATION > src/slave/grace_shutdown.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/28065/diff/ > > > Testing > ------- > > make check (Mac OS 10.9.4, Ubuntu 14.04) > > > Thanks, > > Alexander Rukletsov > >
