> 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? > > Alexander Rukletsov wrote: > 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.
Still, a constant here will keep someone from changing the default to 3 as well and us running into a hard to track down bug. > 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. > > Alexander Rukletsov wrote: > Got it. I think we can leave it as it is, since the name length doesn't > involve line wrapping. In addition to Niklas comment, in other places in the codebase we've also named the variable based on the operation we're doing. This is not a style guideline, but it often makes for fairly readable code with short variable names. To be concrete, in this case, you could name this variable 'parse'. Then, for example, if you're checking if the call to parse was an error or not you'd get code that looks like: if (parse.isError()) { LOG(WARNING) << "Failed to parse: " << parse.error(); } else { shutdownTimeout = parse.get(); ... } - Benjamin ----------------------------------------------------------- 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 > >
