mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 25434: Propagate slave shutdown grace period to Executor and CommandExecutor.
Date Tue, 09 Sep 2014 17:57:47 GMT


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


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