You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexander Rukletsov <al...@mesosphere.io> on 2014/09/08 22:04:01 UTC
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
-----------------------------------------------------------
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
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52666
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25434]
All tests passed.
- Mesos ReviewBot
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 9, 2014, 4:21 p.m., Timothy Chen wrote:
> > src/launcher/executor.cpp, line 682
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line682>
> >
> > I'm not sure I'm really following the logic, I know the levels but I don't know why there is the divide by 2/3 on each level?
Since we have nested grace periods, we would like to guarantee that the parent one is greater than the child one, otherwise the child can be killed by the parent though the shutdown was successful.
> On Sept. 9, 2014, 4:21 p.m., Timothy Chen wrote:
> > src/launcher/executor.cpp, line 664
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line664>
> >
> > I'm not sure we've all agreed to start using C++11 yet?
>
> Niklas Nielsen wrote:
> Good catch - no auto's yet.
My understanding is that we already can: https://mail-archives.apache.org/mod_mbox/mesos-dev/201408.mbox/%3CCA+A2mTsGU2CfcdROo-wPgnd95jy6XQcaNNg6n6MJqYhdnRjB-g@mail.gmail.com%3E
> On Sept. 9, 2014, 4:21 p.m., Timothy Chen wrote:
> > src/exec/exec.cpp, line 738
> > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line738>
> >
> > Why divide by 2 here? It seems like no matter what you're shrinking the timeout?
I'm not shrinking the slave's timeout (the one the user set in flags), rather I set the timeout in Executor to a smaller value. This value is determined according to the slave's timeout.
> On Sept. 9, 2014, 4:21 p.m., Timothy Chen wrote:
> > src/exec/exec.cpp, line 736
> > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line736>
> >
> > let's log when you shrink the timeout.
Makes sense, will do it.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52729
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Sept. 9, 2014, 9:21 a.m., Timothy Chen wrote:
> > src/launcher/executor.cpp, line 664
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line664>
> >
> > I'm not sure we've all agreed to start using C++11 yet?
Good catch - no auto's yet.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52729
-----------------------------------------------------------
On Sept. 9, 2014, 5:54 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 5:54 a.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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 9, 2014, 4:21 p.m., Timothy Chen wrote:
> > src/launcher/executor.cpp, line 682
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line682>
> >
> > I'm not sure I'm really following the logic, I know the levels but I don't know why there is the divide by 2/3 on each level?
>
> Alexander Rukletsov wrote:
> Since we have nested grace periods, we would like to guarantee that the parent one is greater than the child one, otherwise the child can be killed by the parent though the shutdown was successful.
Given that we've had to elaborate this to a review it likely means the comment there needs more substance. And or, if another comment has more substance, we should point folks to that comment in order to understand why something is happening that way (but of course, this has the risk of dangling pointers if/when those other comments move).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52729
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 9, 2014, 4:21 p.m., Timothy Chen wrote:
> > src/exec/exec.cpp, line 738
> > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line738>
> >
> > Why divide by 2 here? It seems like no matter what you're shrinking the timeout?
>
> Alexander Rukletsov wrote:
> I'm not shrinking the slave's timeout (the one the user set in flags), rather I set the timeout in Executor to a smaller value. This value is determined according to the slave's timeout.
Extracted to utility function, dropping the issue.
> On Sept. 9, 2014, 4:21 p.m., Timothy Chen wrote:
> > src/launcher/executor.cpp, line 682
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line682>
> >
> > I'm not sure I'm really following the logic, I know the levels but I don't know why there is the divide by 2/3 on each level?
>
> Alexander Rukletsov wrote:
> Since we have nested grace periods, we would like to guarantee that the parent one is greater than the child one, otherwise the child can be killed by the parent though the shutdown was successful.
>
> Benjamin Hindman wrote:
> Given that we've had to elaborate this to a review it likely means the comment there needs more substance. And or, if another comment has more substance, we should point folks to that comment in order to understand why something is happening that way (but of course, this has the risk of dangling pointers if/when those other comments move).
Extracted to a separate function, added more docs.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52729
-----------------------------------------------------------
On Sept. 18, 2014, 11:03 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 11:03 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 9b973e5
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52729
-----------------------------------------------------------
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment91709>
let's log when you shrink the timeout.
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment91710>
Why divide by 2 here? It seems like no matter what you're shrinking the timeout?
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment91712>
I'm not sure we've all agreed to start using C++11 yet?
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment91713>
I'm not sure I'm really following the logic, I know the levels but I don't know why there is the divide by 2/3 on each level?
- Timothy Chen
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> 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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> 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.
>
> Benjamin Hindman wrote:
> 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();
> ...
> }
Totally agree.
> 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.
>
> Benjamin Hindman wrote:
> 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.
Changed to default value for clarity.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52726
-----------------------------------------------------------
On Sept. 18, 2014, 11:03 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 11:03 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 9b973e5
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> 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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52726
-----------------------------------------------------------
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?
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment91705>
Small nit: We try to keep the variable names short and concise. I would have dropped the 'mesos' prefix.
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment91708>
Till raised an issue with namespace aliasing - did you guys sort that out?
I am not a fan either.
src/slave/constants.hpp
<https://reviews.apache.org/r/25434/#comment91707>
Small bit: max columns for comments are 70: http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/
src/tests/containerizer.cpp
<https://reviews.apache.org/r/25434/#comment91706>
Why 3 seconds?
- Niklas Nielsen
On Sept. 9, 2014, 5:54 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 5:54 a.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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by "Timothy St. Clair" <ts...@redhat.com>.
> On Sept. 11, 2014, 3:45 p.m., Timothy St. Clair wrote:
> > src/exec/exec.cpp, line 82
> > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line82>
> >
> > Maybe I'm missing something, but is there a reason we don't check before a delay? If ShutdownProcess is spawned, it's possible all other processes in the group have already cleaned up.
>
> Alexander Rukletsov wrote:
> Good questions. Can this be related to non-native bindings?
Perhaps we can open a separate JIRA on this issue as it was here before.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review53038
-----------------------------------------------------------
On Sept. 18, 2014, 11:03 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 11:03 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 9b973e5
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 11, 2014, 3:45 p.m., Timothy St. Clair wrote:
> > src/tests/containerizer.cpp, line 113
> > <https://reviews.apache.org/r/25434/diff/2/?file=683955#file683955line113>
> >
> > Manifest constant please.
>
> Alexander Rukletsov wrote:
> Ok.
Use default.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review53038
-----------------------------------------------------------
On Sept. 18, 2014, 11:03 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 11:03 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 9b973e5
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 11, 2014, 3:45 p.m., Timothy St. Clair wrote:
> > src/exec/exec.cpp, line 82
> > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line82>
> >
> > Maybe I'm missing something, but is there a reason we don't check before a delay? If ShutdownProcess is spawned, it's possible all other processes in the group have already cleaned up.
>
> Alexander Rukletsov wrote:
> Good questions. Can this be related to non-native bindings?
>
> Timothy St. Clair wrote:
> Perhaps we can open a separate JIRA on this issue as it was here before.
https://issues.apache.org/jira/browse/MESOS-1823
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review53038
-----------------------------------------------------------
On Sept. 18, 2014, 11:03 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 11:03 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 9b973e5
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 11, 2014, 3:45 p.m., Timothy St. Clair wrote:
> > src/exec/exec.cpp, line 82
> > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line82>
> >
> > Maybe I'm missing something, but is there a reason we don't check before a delay? If ShutdownProcess is spawned, it's possible all other processes in the group have already cleaned up.
Good questions. Can this be related to non-native bindings?
> On Sept. 11, 2014, 3:45 p.m., Timothy St. Clair wrote:
> > src/exec/exec.cpp, line 725
> > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line725>
> >
> > Does it make more sense to provide a sane default and log in the case of bad config. Doesn't CHECK_SOME bail?
Agreed, will do so.
> On Sept. 11, 2014, 3:45 p.m., Timothy St. Clair wrote:
> > src/launcher/executor.cpp, line 628
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line628>
> >
> > Doesn't the flags load export override for env vars that are on the commend line? I would keep the original comment.
> >
> > e.g.
> >
> > cat /etc/mesos/mesos-slave-env.sh
> > export MESOS_containerizers=docker,mesos
I would rather separate them, but you are right, I'll change the order for clarity.
> On Sept. 11, 2014, 3:45 p.m., Timothy St. Clair wrote:
> > src/launcher/executor.cpp, line 679
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line679>
> >
> > imho the shutdown timeout logic should be part of a utility function, vs. re-creating in different locations, with subtle differences.
Agreed.
> On Sept. 11, 2014, 3:45 p.m., Timothy St. Clair wrote:
> > src/tests/containerizer.cpp, line 113
> > <https://reviews.apache.org/r/25434/diff/2/?file=683955#file683955line113>
> >
> > Manifest constant please.
Ok.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review53038
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by "Timothy St. Clair" <ts...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review53038
-----------------------------------------------------------
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment92392>
Maybe I'm missing something, but is there a reason we don't check before a delay? If ShutdownProcess is spawned, it's possible all other processes in the group have already cleaned up.
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment92395>
Does it make more sense to provide a sane default and log in the case of bad config. Doesn't CHECK_SOME bail?
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment92399>
Doesn't the flags load export override for env vars that are on the commend line? I would keep the original comment.
e.g.
cat /etc/mesos/mesos-slave-env.sh
export MESOS_containerizers=docker,mesos
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment92400>
I see we all agree here ;-)
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment92402>
Same logic as above, do we want to bail on bad config or sane default? Imagine thousands of computers and some depoy script gets botched.
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment92404>
imho the shutdown timeout logic should be part of a utility function, vs. re-creating in different locations, with subtle differences.
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/25434/#comment92407>
This is more of a general comment for the gallery, but as signatures keep growing at some point we need to determine a limit and either create a utility class | struct to encapsulate.
This appears to be a general theme.
src/tests/containerizer.cpp
<https://reviews.apache.org/r/25434/#comment92408>
Manifest constant please.
- Timothy St. Clair
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52727
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25434]
All tests passed.
- Mesos ReviewBot
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52740
-----------------------------------------------------------
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment91728>
Got it, didn't see the passing the shutdownTimeout down the next level line.
- Timothy Chen
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/exec/exec.cpp, lines 75-76
> > <https://reviews.apache.org/r/25434/diff/4/?file=709150#file709150line75>
> >
> > Why Java executors? I am not sure I understand this comment.
This is what BenH pointed me at. Since Java has garbage collection, an Executor written in java may be garbage collected during the graceful shutdown period. We spawn a separate process exactly to target this problem.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/slave/constants.hpp, lines 51-59
> > <https://reviews.apache.org/r/25434/diff/4/?file=709152#file709152line51>
> >
> > Can we add something similar to the flags help text? The flag is already a bit misleading, so it would be helpful to expand on the new funtionality there.
Agreed, definitely makes sense.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/slave/utils.hpp, lines 29-34
> > <https://reviews.apache.org/r/25434/diff/4/?file=709160#file709160line29>
> >
> > Can you include an example? How about the sequence charts from the JIRA? The logic of adjustShutdownTimeout is still not crystal clear to me.
> > To put it in another way; it is hard for me to tell whether we cover all corner cases.
The approach I took gives two guarantees: a timeout is always nonnegative and not greater than the parent one. We can get a situation when the timeout at one or more levels is ridiculously small, which effectively disables the graceful shutdown. Do you have any specific concerns?
I have added the sequence chart and expanded the comment a bit.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/slave/utils.hpp, line 35
> > <https://reviews.apache.org/r/25434/diff/4/?file=709160#file709160line35>
> >
> > How about 'getShutdownTimeout'? It is not really adjusting, but rather partition the total timeout into sub-timeouts - correct?
> >
> > Also, we can hide the implementation adjustShutdownTimeout in utils.cpp so we only export adjustExecutorShutdownTimeout and adjustCommandExecutorShutdownTimeout (which could be called 'getExecutorShutdownTimeout'?)
'getShutdownTimeout' implies the timeout is not calculated on the fly based on input parameters. 'getShutdownTimeoutForLevel' is better, but too long. How about 'calculateShutdownTimeout' for the basic function and 'get...' for the specific ones? I would also leave the basic function publicly accessible for potential use in other places and levels.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/slave/utils.cpp, line 42
> > <https://reviews.apache.org/r/25434/diff/4/?file=709161#file709161line42>
> >
> > Do we want to have checks/asserts to ensure we have positive/non-zero timeouts?
I think currently it's difficult to supply a negative duration, but agree, let's do it for the future.
> On Oct. 3, 2014, 5:25 p.m., Niklas Nielsen wrote:
> > src/tests/mesos.hpp, lines 865-868
> > <https://reviews.apache.org/r/25434/diff/4/?file=709163#file709163line865>
> >
> > You only use this in MesosExecutorForceShutdown - how about moving it to slave_tests.cpp instead?
> >
> > How about using 'statusMatchesTask' or 'statusHasTaskId'. 'relatedTo' seems a bit informal/imprecise.
Changed name to `statusMatchesTask ()`. We do not have utility functions in `slave_tests.cpp`, and the predicate is rather generic, so I would leave it in the `tests/mesos.hpp` where it can be found faster by the next one who needs the same functionality.
I remove this test from this request and move it to the newly created [JIRA ticket](https://issues.apache.org/jira/browse/MESOS-1871).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55348
-----------------------------------------------------------
On Oct. 2, 2014, 7:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55348
-----------------------------------------------------------
Hi Alex,
Sorry for the tardy reply on this review. I have left a few high-level comments - think that Tim touched upon most of the style issues.
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment95718>
Why Java executors? I am not sure I understand this comment.
src/slave/constants.hpp
<https://reviews.apache.org/r/25434/#comment95726>
Can we add something similar to the flags help text? The flag is already a bit misleading, so it would be helpful to expand on the new funtionality there.
src/slave/utils.hpp
<https://reviews.apache.org/r/25434/#comment95715>
Can you include an example? How about the sequence charts from the JIRA? The logic of adjustShutdownTimeout is still not crystal clear to me.
To put it in another way; it is hard for me to tell whether we cover all corner cases.
src/slave/utils.hpp
<https://reviews.apache.org/r/25434/#comment95714>
How about 'getShutdownTimeout'? It is not really adjusting, but rather partition the total timeout into sub-timeouts - correct?
Also, we can hide the implementation adjustShutdownTimeout in utils.cpp so we only export adjustExecutorShutdownTimeout and adjustCommandExecutorShutdownTimeout (which could be called 'getExecutorShutdownTimeout'?)
src/slave/utils.cpp
<https://reviews.apache.org/r/25434/#comment95716>
Do we want to have checks/asserts to ensure we have positive/non-zero timeouts?
src/tests/mesos.hpp
<https://reviews.apache.org/r/25434/#comment95717>
You only use this in MesosExecutorForceShutdown - how about moving it to slave_tests.cpp instead?
How about using 'statusMatchesTask' or 'statusHasTaskId'. 'relatedTo' seems a bit informal/imprecise.
- Niklas Nielsen
On Oct. 2, 2014, 12:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 12:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 2, 2014, 12:49 a.m., Timothy Chen wrote:
> > src/slave/utils.cpp, line 39
> > <https://reviews.apache.org/r/25434/diff/4/?file=709161#file709161line39>
> >
> > Log the timeout value
Both the one that is too small and the effective one.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55196
-----------------------------------------------------------
On Oct. 2, 2014, 12:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 12:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Oct. 2, 2014, 7:49 a.m., Timothy Chen wrote:
> > src/exec/exec.cpp, line 739
> > <https://reviews.apache.org/r/25434/diff/4/?file=709150#file709150line739>
> >
> > I'm not sure this should be at the WARNING level, as it's not really expected to have it set all the time. IMHO it should be INFO
We actually expect it to be set explicitly in order to avoid situations where slave uses one value and executor another.
> On Oct. 2, 2014, 7:49 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 1023
> > <https://reviews.apache.org/r/25434/diff/4/?file=709164#file709164line1023>
> >
> > Times(1) is default, can ignore the call.
To the best of my knowledge, we always explicitly write `Times(1)` in such cases.
> On Oct. 2, 2014, 7:49 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 1061
> > <https://reviews.apache.org/r/25434/diff/4/?file=709164#file709164line1061>
> >
> > EXPECT_TRUE
Good catch!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55196
-----------------------------------------------------------
On Oct. 2, 2014, 7:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 2, 2014, 7:49 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 1023
> > <https://reviews.apache.org/r/25434/diff/4/?file=709164#file709164line1023>
> >
> > Times(1) is default, can ignore the call.
>
> Alexander Rukletsov wrote:
> To the best of my knowledge, we always explicitly write `Times(1)` in such cases.
:) Well I got commented on about it by other committers, so I'll let you decide what to do here...
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55196
-----------------------------------------------------------
On Oct. 7, 2014, 3:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 3:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am cff38ce
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 16f0cc2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.4, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55196
-----------------------------------------------------------
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment95590>
I'm not sure this should be at the WARNING level, as it's not really expected to have it set all the time. IMHO it should be INFO
src/slave/utils.cpp
<https://reviews.apache.org/r/25434/#comment95591>
I think the usual style is to move mesos::internal::slave::SHUTDOWN_TIMEOUT_DELTA *
to the next line
src/slave/utils.cpp
<https://reviews.apache.org/r/25434/#comment95592>
Log the timeout value
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/25434/#comment95593>
Times(1) is default, can ignore the call.
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/25434/#comment95594>
EXPECT_TRUE
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/25434/#comment95595>
ditto
- Timothy Chen
On Oct. 2, 2014, 7:17 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:17 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 27c42df
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 32e51d2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/mesos.hpp 957e223
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
> WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review55662
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25434]
All tests passed.
- Mesos ReviewBot
On Oct. 7, 2014, 3:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 3:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am cff38ce
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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 9a29489
> src/slave/containerizer/external_containerizer.cpp efbc68f
> src/slave/containerizer/mesos/containerizer.cpp 9d08329
> src/slave/flags.hpp 16f0cc2
> src/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
> src/tests/slave_tests.cpp 69be28f
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.4, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/
-----------------------------------------------------------
(Updated Oct. 7, 2014, 3:54 p.m.)
Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
Changes
-------
Address issues.
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 (updated)
-----
src/Makefile.am cff38ce
src/exec/exec.cpp e15f834
src/launcher/executor.cpp cbc8750
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 9a29489
src/slave/containerizer/external_containerizer.cpp efbc68f
src/slave/containerizer/mesos/containerizer.cpp 9d08329
src/slave/flags.hpp 16f0cc2
src/slave/utils.hpp PRE-CREATION
src/slave/utils.cpp PRE-CREATION
src/tests/containerizer.cpp a17e1e0
src/tests/slave_tests.cpp 69be28f
Diff: https://reviews.apache.org/r/25434/diff/
Testing (updated)
-------
make check (Mac OS 10.9.4, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/
-----------------------------------------------------------
(Updated Oct. 2, 2014, 7:17 a.m.)
Review request for mesos, Benjamin Hindman, 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/Makefile.am 27c42df
src/exec/exec.cpp e15f834
src/launcher/executor.cpp cbc8750
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 9a29489
src/slave/containerizer/external_containerizer.cpp efbc68f
src/slave/containerizer/mesos/containerizer.cpp 9d08329
src/slave/flags.hpp 32e51d2
src/slave/utils.hpp PRE-CREATION
src/slave/utils.cpp PRE-CREATION
src/tests/containerizer.cpp a17e1e0
src/tests/mesos.hpp 957e223
src/tests/slave_tests.cpp 69be28f
Diff: https://reviews.apache.org/r/25434/diff/
Testing (updated)
-------
make check (OS X 10.9.4)
WIP: digging into the failure of the SlaveTest.MesosExecutorForceShutdown test revealed an issue with signal escalation in CommandExecutor. That needs more time to be resolved.
Thanks,
Alexander Rukletsov
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/
-----------------------------------------------------------
(Updated Sept. 30, 2014, 2:16 p.m.)
Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
Changes
-------
Add tests and rebase.
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 (updated)
-----
src/Makefile.am 27c42df
src/exec/exec.cpp e15f834
src/launcher/executor.cpp cbc8750
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 9a29489
src/slave/containerizer/external_containerizer.cpp efbc68f
src/slave/containerizer/mesos/containerizer.cpp 9d08329
src/slave/flags.hpp 32e51d2
src/slave/utils.hpp PRE-CREATION
src/slave/utils.cpp PRE-CREATION
src/tests/containerizer.cpp a17e1e0
src/tests/mesos.hpp 957e223
src/tests/slave_tests.cpp 69be28f
Diff: https://reviews.apache.org/r/25434/diff/
Testing
-------
make check (OS X 10.9.4)
Thanks,
Alexander Rukletsov
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review53816
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25434]
All tests passed.
- Mesos ReviewBot
On Sept. 18, 2014, 11:03 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 11:03 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 9b973e5
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/
-----------------------------------------------------------
(Updated Sept. 18, 2014, 11:03 a.m.)
Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Timothy St. Clair.
Changes
-------
Address mentioned issues except tests.
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 (updated)
-----
src/Makefile.am 9b973e5
src/exec/exec.cpp e15f834
src/launcher/executor.cpp cbc8750
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/slave/utils.hpp PRE-CREATION
src/slave/utils.cpp PRE-CREATION
src/tests/containerizer.cpp a17e1e0
Diff: https://reviews.apache.org/r/25434/diff/
Testing (updated)
-------
make check (OS X 10.9.4)
Thanks,
Alexander Rukletsov
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/launcher/executor.cpp, lines 654-656
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line654>
> >
> > I'd rather not introduce namespace aliases, can we not put all of this in mesos::internal?
>
> Alexander Rukletsov wrote:
> Looks like "namespace alias"-pro are in minority! Ok, I'll remove them here and start a JIRA ticket to update the C++ style guide.
>
> Benjamin Hindman wrote:
> So stoked to hear you're also going to update the style guide. Really appreciate following through on that.
[Done](https://issues.apache.org/jira/browse/MESOS-1793).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52750
-----------------------------------------------------------
On Sept. 18, 2014, 11:03 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25434/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 11:03 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, 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/Makefile.am 9b973e5
> src/exec/exec.cpp e15f834
> src/launcher/executor.cpp cbc8750
> 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/slave/utils.hpp PRE-CREATION
> src/slave/utils.cpp PRE-CREATION
> src/tests/containerizer.cpp a17e1e0
>
> Diff: https://reviews.apache.org/r/25434/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/slave/constants.hpp, line 53
> > <https://reviews.apache.org/r/25434/diff/2/?file=683947#file683947line53>
> >
> > What is the 'base executor' versus the 'command executor'?
We have Executor (lives in src/exec/exec.cpp) and CommandExecutor aka mesos-executor (lives in src/launcher/executor.cpp). I find "executor" too vague and use "base executor" to stress out I mean the one that lives in exec.cpp. Is there a convention about naming these folks?
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/exec/exec.cpp, lines 732-734
> > <https://reviews.apache.org/r/25434/diff/2/?file=683945#file683945line732>
> >
> > Great to see a comment giving the _what_, but I'm left wondering the _why_? Why are you making the timeout shorter than the slave one? Even if it's obvious to you this is something that would be great to leave for posterity!
Agree, I will give more background here.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/launcher/executor.cpp, line 76
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line76>
> >
> > Minor style nit, please move all these indents to +4 on a newline after 'CommandExecutorProcess('. Same with the 'CommandExecutor' constructor below. Thanks Alex!
Ok.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/launcher/executor.cpp, line 510
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line510>
> >
> > As long as you're editing up here, let's clean this up with s/string/const string&/ please, thanks!
Ok.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/launcher/executor.cpp, line 513
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line513>
> >
> > Please move 'override' to newline too, thanks!
Ok.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/launcher/executor.cpp, lines 654-656
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line654>
> >
> > I'd rather not introduce namespace aliases, can we not put all of this in mesos::internal?
Looks like "namespace alias"-pro are in minority! Ok, I'll remove them here and start a JIRA ticket to update the C++ style guide.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/launcher/executor.cpp, line 664
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line664>
> >
> > Why not call it 'value' like we do in the code above?
Sure.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52750
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/slave/constants.hpp, line 53
> > <https://reviews.apache.org/r/25434/diff/2/?file=683947#file683947line53>
> >
> > What is the 'base executor' versus the 'command executor'?
>
> Alexander Rukletsov wrote:
> We have Executor (lives in src/exec/exec.cpp) and CommandExecutor aka mesos-executor (lives in src/launcher/executor.cpp). I find "executor" too vague and use "base executor" to stress out I mean the one that lives in exec.cpp. Is there a convention about naming these folks?
>
> Benjamin Hindman wrote:
> Ah, I see. Well, CommandExecutor is just an instance of an executor and actually uses the code from exec.cpp just like all current executors do (that use libmesos). So there aren't actually two executors (base and command), just one, and they all use exec.cpp (if they use libmesos). Does that make sense?
>
> Alexander Rukletsov wrote:
> Sorry, I was inexact in my comment. Indeed, there is only one executor, but two libprocess processes (where all most of the work is done). Here is what we have:
> Executor <-> ExecutorProcess (I call them both "base executor", though "base executor process" is more correct)
> CommandExecutor <-> CommandExecutorProcess
> The OS process where CommandExecutor lives instantiates also the driver, together it looks like this:
> _______________________________________________
> MesosExecutorDriver
> * ExecutorProcess
> |
> V
> * CommandExecutor <-> CommandExecutorProcess
> |
> V
> task
> _______________________________________________
>
> My aim was to explain that there is a "wrapper" around the CommandExecutorProcess which has its own shutdown period. For simplicity I called this "wrapper" (which is ExecutorProcess and a bit MesosExecutorDriver) "base executor". However, it looks like my terminology is not good enough and maybe even misleading. What terms would you suggest, Ben?
IMHO we should just use the terms ExecutorProcess, MesosExecutorDriver, etc. If you want to alias them within that comment then I'd suggest defining the alias (as you've done for me here) and then using that alias in the comment. That being said, my hunch is that you'll get more milage just using the class names. This will also make renaming/refactoring easier as code searches will grab these comments too.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52750
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/slave/constants.hpp, line 53
> > <https://reviews.apache.org/r/25434/diff/2/?file=683947#file683947line53>
> >
> > What is the 'base executor' versus the 'command executor'?
>
> Alexander Rukletsov wrote:
> We have Executor (lives in src/exec/exec.cpp) and CommandExecutor aka mesos-executor (lives in src/launcher/executor.cpp). I find "executor" too vague and use "base executor" to stress out I mean the one that lives in exec.cpp. Is there a convention about naming these folks?
Ah, I see. Well, CommandExecutor is just an instance of an executor and actually uses the code from exec.cpp just like all current executors do (that use libmesos). So there aren't actually two executors (base and command), just one, and they all use exec.cpp (if they use libmesos). Does that make sense?
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/launcher/executor.cpp, lines 654-656
> > <https://reviews.apache.org/r/25434/diff/2/?file=683946#file683946line654>
> >
> > I'd rather not introduce namespace aliases, can we not put all of this in mesos::internal?
>
> Alexander Rukletsov wrote:
> Looks like "namespace alias"-pro are in minority! Ok, I'll remove them here and start a JIRA ticket to update the C++ style guide.
So stoked to hear you're also going to update the style guide. Really appreciate following through on that.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52750
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/slave/constants.hpp, line 53
> > <https://reviews.apache.org/r/25434/diff/2/?file=683947#file683947line53>
> >
> > What is the 'base executor' versus the 'command executor'?
>
> Alexander Rukletsov wrote:
> We have Executor (lives in src/exec/exec.cpp) and CommandExecutor aka mesos-executor (lives in src/launcher/executor.cpp). I find "executor" too vague and use "base executor" to stress out I mean the one that lives in exec.cpp. Is there a convention about naming these folks?
>
> Benjamin Hindman wrote:
> Ah, I see. Well, CommandExecutor is just an instance of an executor and actually uses the code from exec.cpp just like all current executors do (that use libmesos). So there aren't actually two executors (base and command), just one, and they all use exec.cpp (if they use libmesos). Does that make sense?
>
> Alexander Rukletsov wrote:
> Sorry, I was inexact in my comment. Indeed, there is only one executor, but two libprocess processes (where all most of the work is done). Here is what we have:
> Executor <-> ExecutorProcess (I call them both "base executor", though "base executor process" is more correct)
> CommandExecutor <-> CommandExecutorProcess
> The OS process where CommandExecutor lives instantiates also the driver, together it looks like this:
> _______________________________________________
> MesosExecutorDriver
> * ExecutorProcess
> |
> V
> * CommandExecutor <-> CommandExecutorProcess
> |
> V
> task
> _______________________________________________
>
> My aim was to explain that there is a "wrapper" around the CommandExecutorProcess which has its own shutdown period. For simplicity I called this "wrapper" (which is ExecutorProcess and a bit MesosExecutorDriver) "base executor". However, it looks like my terminology is not good enough and maybe even misleading. What terms would you suggest, Ben?
>
> Benjamin Hindman wrote:
> IMHO we should just use the terms ExecutorProcess, MesosExecutorDriver, etc. If you want to alias them within that comment then I'd suggest defining the alias (as you've done for me here) and then using that alias in the comment. That being said, my hunch is that you'll get more milage just using the class names. This will also make renaming/refactoring easier as code searches will grab these comments too.
Ok, agreed.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52750
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 9, 2014, 5:50 p.m., Benjamin Hindman wrote:
> > src/slave/constants.hpp, line 53
> > <https://reviews.apache.org/r/25434/diff/2/?file=683947#file683947line53>
> >
> > What is the 'base executor' versus the 'command executor'?
>
> Alexander Rukletsov wrote:
> We have Executor (lives in src/exec/exec.cpp) and CommandExecutor aka mesos-executor (lives in src/launcher/executor.cpp). I find "executor" too vague and use "base executor" to stress out I mean the one that lives in exec.cpp. Is there a convention about naming these folks?
>
> Benjamin Hindman wrote:
> Ah, I see. Well, CommandExecutor is just an instance of an executor and actually uses the code from exec.cpp just like all current executors do (that use libmesos). So there aren't actually two executors (base and command), just one, and they all use exec.cpp (if they use libmesos). Does that make sense?
Sorry, I was inexact in my comment. Indeed, there is only one executor, but two libprocess processes (where all most of the work is done). Here is what we have:
Executor <-> ExecutorProcess (I call them both "base executor", though "base executor process" is more correct)
CommandExecutor <-> CommandExecutorProcess
The OS process where CommandExecutor lives instantiates also the driver, together it looks like this:
_______________________________________________
MesosExecutorDriver
* ExecutorProcess
|
V
* CommandExecutor <-> CommandExecutorProcess
|
V
task
_______________________________________________
My aim was to explain that there is a "wrapper" around the CommandExecutorProcess which has its own shutdown period. For simplicity I called this "wrapper" (which is ExecutorProcess and a bit MesosExecutorDriver) "base executor". However, it looks like my terminology is not good enough and maybe even misleading. What terms would you suggest, Ben?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52750
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52750
-----------------------------------------------------------
Great seeing new folks submitting reviews! Just a drive by here, Niklas will still shepherd this (unless I'm mistaken and you have a arranged with a different shepherd).
src/exec/exec.cpp
<https://reviews.apache.org/r/25434/#comment91748>
Great to see a comment giving the _what_, but I'm left wondering the _why_? Why are you making the timeout shorter than the slave one? Even if it's obvious to you this is something that would be great to leave for posterity!
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment91749>
Minor style nit, please move all these indents to +4 on a newline after 'CommandExecutorProcess('. Same with the 'CommandExecutor' constructor below. Thanks Alex!
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment91751>
As long as you're editing up here, let's clean this up with s/string/const string&/ please, thanks!
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment91753>
Please move 'override' to newline too, thanks!
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment91754>
I'd rather not introduce namespace aliases, can we not put all of this in mesos::internal?
src/launcher/executor.cpp
<https://reviews.apache.org/r/25434/#comment91755>
Why not call it 'value' like we do in the code above?
src/slave/constants.hpp
<https://reviews.apache.org/r/25434/#comment91771>
What is the 'base executor' versus the 'command executor'?
- Benjamin Hindman
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Address Till's comments.
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 (updated)
-----
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
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On Sept. 9, 2014, 9:33 a.m., Till Toenshoff wrote:
> > src/slave/constants.hpp, lines 51-57
> > <https://reviews.apache.org/r/25434/diff/1/?file=682522#file682522line51>
> >
> > 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).
True, I have expanded the comment to the slave flag.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25434/#review52689
-----------------------------------------------------------
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
>
>
Re: Review Request 25434: Propagate slave shutdown grace period to
Executor and CommandExecutor.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
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
>
>