You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Steve Niemitz <st...@tellapart.com> on 2015/02/25 21:17:07 UTC
Review Request 31423: Stop the announcer and status checkers before
starting to kill the runners
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/
-----------------------------------------------------------
Review request for Aurora.
Repository: aurora
Description
-------
Stop the announcer and status checkers before starting to kill the runners
Diffs
-----
src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
Diff: https://reviews.apache.org/r/31423/diff/
Testing
-------
Thanks,
Steve Niemitz
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74087
-----------------------------------------------------------
Master (895d03b) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 25, 2015, 8:17 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2015, 8:17 p.m.)
>
>
> Review request for Aurora.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74100
-----------------------------------------------------------
Ship it!
I think it is logical to shut down the status checkers before shutting down the task.
- Zameer Manji
On Feb. 25, 2015, 1:22 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2015, 1:22 p.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74618
-----------------------------------------------------------
Ship it!
Ship It!
- Brian Wickman
On Feb. 26, 2015, 3:20 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 3:20 p.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Steve Niemitz <st...@tellapart.com>.
> On Feb. 27, 2015, 11:05 p.m., Joe Smith wrote:
> > Thought this over- the benefits to overall reliability are significant and applications should still be motivated to be resilient to the edge case of 'ZK registration but application unresponsive'.
> >
> > Thanks Steve!
:D
- Steve
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74620
-----------------------------------------------------------
On Feb. 26, 2015, 3:20 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 3:20 p.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Joe Smith <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74620
-----------------------------------------------------------
Ship it!
Thought this over- the benefits to overall reliability are significant and applications should still be motivated to be resilient to the edge case of 'ZK registration but application unresponsive'.
Thanks Steve!
src/test/python/apache/aurora/executor/test_thermos_executor.py
<https://reviews.apache.org/r/31423/#comment121243>
good idea
- Joe Smith
On Feb. 26, 2015, 7:20 a.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 7:20 a.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Brian Wickman <wi...@apache.org>.
> On March 3, 2015, 3:26 p.m., Steve Niemitz wrote:
> > Is there anything else here? Is this ready to be merged?
Yup, on it.
- Brian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74958
-----------------------------------------------------------
On Feb. 26, 2015, 3:20 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 3:20 p.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Steve Niemitz <st...@tellapart.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74958
-----------------------------------------------------------
Is there anything else here? Is this ready to be merged?
- Steve Niemitz
On Feb. 26, 2015, 3:20 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 3:20 p.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74277
-----------------------------------------------------------
Ship it!
Master (938144c) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 26, 2015, 3:20 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 3:20 p.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Steve Niemitz <st...@tellapart.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/
-----------------------------------------------------------
(Updated Feb. 26, 2015, 3:20 p.m.)
Review request for Aurora, Brian Wickman and Zameer Manji.
Changes
-------
Made the style checker happy
Repository: aurora
Description
-------
Stop the announcer and status checkers before starting to kill the runners.
This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
Diffs (updated)
-----
src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
Diff: https://reviews.apache.org/r/31423/diff/
Testing
-------
We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
I couldn't observe any other side effects either.
Thanks,
Steve Niemitz
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Steve Niemitz <st...@tellapart.com>.
> On Feb. 26, 2015, 3:38 a.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/executor/test_thermos_executor.py, lines 384-386
> > <https://reviews.apache.org/r/31423/diff/2/?file=877156#file877156line384>
> >
> > You should be able to use contextlib.nested for this:
> >
> > with contextlib.nested(
> > temporary_dir(),
> > mock.patch.object(...),
> > mock.patch.object(...)) as (
> > checkpoint_root,
> > status_check_stop,
> > runner_stop):
> >
> > # use all the things
> >
> > Alternately, I think you can just wrap the with in parens as well, since we're pinned to python2.7 now (though a quick scan of the code doesn't reveal any place that we do that, but does show lots of places where we use contextlib.nested).
I tried parens and they don't work with "with" for whatever reason (syntax error). I tried to avoid contextlib.nested because it's deprecated as of 2.7, but I can use that if you guys think its ok.
> On Feb. 26, 2015, 3:38 a.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/executor/test_thermos_executor.py, line 398
> > <https://reviews.apache.org/r/31423/diff/2/?file=877156#file877156line398>
> >
> > Is it worth being explicit and passing any_order=False (even though it's the default), just to make it clear that the order is important?
> >
> > I had to look it up to see what the behavior was, maybe for people more familiar with python/mock would know right away, but it's not abundantly clear.
:thumbsup:
- Steve
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74202
-----------------------------------------------------------
On Feb. 26, 2015, 2:14 a.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 2:14 a.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Joshua Cohen <jc...@twopensource.com>.
> On Feb. 26, 2015, 3:38 a.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/executor/test_thermos_executor.py, lines 384-386
> > <https://reviews.apache.org/r/31423/diff/2/?file=877156#file877156line384>
> >
> > You should be able to use contextlib.nested for this:
> >
> > with contextlib.nested(
> > temporary_dir(),
> > mock.patch.object(...),
> > mock.patch.object(...)) as (
> > checkpoint_root,
> > status_check_stop,
> > runner_stop):
> >
> > # use all the things
> >
> > Alternately, I think you can just wrap the with in parens as well, since we're pinned to python2.7 now (though a quick scan of the code doesn't reveal any place that we do that, but does show lots of places where we use contextlib.nested).
>
> Steve Niemitz wrote:
> I tried parens and they don't work with "with" for whatever reason (syntax error). I tried to avoid contextlib.nested because it's deprecated as of 2.7, but I can use that if you guys think its ok.
Huh, how about that, seems like a genuine wart that they don't seem interested in fixing: http://bugs.python.org/issue12782
Anyway, the tests are littered with contextlib.nested, so I say go for it!
- Joshua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74202
-----------------------------------------------------------
On Feb. 26, 2015, 2:14 a.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 2:14 a.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74202
-----------------------------------------------------------
src/test/python/apache/aurora/executor/test_thermos_executor.py
<https://reviews.apache.org/r/31423/#comment120720>
You should be able to use contextlib.nested for this:
with contextlib.nested(
temporary_dir(),
mock.patch.object(...),
mock.patch.object(...)) as (
checkpoint_root,
status_check_stop,
runner_stop):
# use all the things
Alternately, I think you can just wrap the with in parens as well, since we're pinned to python2.7 now (though a quick scan of the code doesn't reveal any place that we do that, but does show lots of places where we use contextlib.nested).
src/test/python/apache/aurora/executor/test_thermos_executor.py
<https://reviews.apache.org/r/31423/#comment120721>
Is it worth being explicit and passing any_order=False (even though it's the default), just to make it clear that the order is important?
I had to look it up to see what the behavior was, maybe for people more familiar with python/mock would know right away, but it's not abundantly clear.
- Joshua Cohen
On Feb. 26, 2015, 2:14 a.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 2:14 a.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74191
-----------------------------------------------------------
Master (9442e08) is red with this patch.
./build-support/jenkins/build.sh
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py (namespace package)
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py (namespace package)
Installing /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.util-0.3.0-py2.7-nspkg.pth
Running setup.py install for twitter.common.log
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py (namespace package)
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py (namespace package)
Installing /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.log-0.3.0-py2.7-nspkg.pth
Running setup.py install for twitter.common.process
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py (namespace package)
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py (namespace package)
Installing /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.process-0.3.0-py2.7-nspkg.pth
Running setup.py install for gitdb
building 'gitdb._perf' extension
x86_64-linux-gnu-gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fPIC -Igitdb -I/usr/include/python2.7 -c gitdb/_fun.c -o build/temp.linux-x86_64-2.7/gitdb/_fun.o
x86_64-linux-gnu-gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fPIC -Igitdb -I/usr/include/python2.7 -c gitdb/_delta_apply.c -o build/temp.linux-x86_64-2.7/gitdb/_delta_apply.o
x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -D_FORTIFY_SOURCE=2 -g -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/gitdb/_fun.o build/temp.linux-x86_64-2.7/gitdb/_delta_apply.o -o build/lib.linux-x86_64-2.7/gitdb/_perf.so
Running setup.py install for twitter.common.app
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py (namespace package)
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py (namespace package)
Installing /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.app-0.3.0-py2.7-nspkg.pth
Running setup.py install for GitPython
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/local/lib/python2.7/site-packages/setuptools/dist.py:292: UserWarning: The version specified ('0.3.2 RC1') is an invalid version, this may not work as expected with newer versions of setuptools, pip, and PyPI. Please see PEP 440 for more details.
"details." % self.metadata.version
Running setup.py install for pep8
Installing pep8 script to /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/bin
Running setup.py install for pyflakes
Installing pyflakes script to /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/bin
Running setup.py install for twitter.checkstyle
Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py (namespace package)
Installing /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.checkstyle-0.1.0-py2.7-nspkg.pth
Installing twitterstyle script to /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/bin
Successfully installed GitPython-0.3.2rc1 gitdb-0.6.4 pep8-1.4.5 pyflakes-0.7.2 smmap-0.9.0 twitter.checkstyle-0.1.0 twitter.common.app-0.3.0 twitter.common.collections-0.3.0 twitter.common.contextutil-0.3.0 twitter.common.dirutil-0.3.0 twitter.common.lang-0.3.0 twitter.common.log-0.3.0 twitter.common.options-0.3.0 twitter.common.process-0.3.0 twitter.common.string-0.3.0 twitter.common.util-0.3.0
T201:ERROR src/test/python/apache/aurora/executor/test_thermos_executor.py:384-386 Line has trailing slashes.
| with temporary_dir() as checkpoint_root, \
| mock.patch.object(ChainedStatusChecker, 'stop') as status_check_stop, \
| mock.patch.object(ThermosTaskRunner, 'stop') as runner_stop:
T201:ERROR src/test/python/apache/aurora/executor/test_thermos_executor.py:385 Line has trailing slashes.
| mock.patch.object(ChainedStatusChecker, 'stop') as status_check_stop, \
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 26, 2015, 2:14 a.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 2:14 a.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
> src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
> src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Steve Niemitz <st...@tellapart.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/
-----------------------------------------------------------
(Updated Feb. 26, 2015, 2:14 a.m.)
Review request for Aurora, Brian Wickman and Zameer Manji.
Changes
-------
This fails the stylechecker because of the multiline with clause, but there's no better way to do it that I can see?
Anyways, this adds a unit test to enforce the call order to be status_checker.stop() -> runner.stop()
Repository: aurora
Description
-------
Stop the announcer and status checkers before starting to kill the runners.
This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
Diffs (updated)
-----
src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222
src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28
Diff: https://reviews.apache.org/r/31423/diff/
Testing
-------
We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
I couldn't observe any other side effects either.
Thanks,
Steve Niemitz
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Zameer Manji <zm...@apache.org>.
> On Feb. 25, 2015, 2:15 p.m., Joshua Cohen wrote:
> > Is it worth adding test coverage for this shutdown ordering?
I don't think mock allows us to test for order. Steve can you take a quick look and tell us if it is possible?
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74104
-----------------------------------------------------------
On Feb. 25, 2015, 1:22 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2015, 1:22 p.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/#review74104
-----------------------------------------------------------
Is it worth adding test coverage for this shutdown ordering?
- Joshua Cohen
On Feb. 25, 2015, 9:22 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31423/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2015, 9:22 p.m.)
>
>
> Review request for Aurora, Brian Wickman and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Stop the announcer and status checkers before starting to kill the runners.
>
> This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
>
> Diff: https://reviews.apache.org/r/31423/diff/
>
>
> Testing
> -------
>
> We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
>
> I couldn't observe any other side effects either.
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Steve Niemitz <st...@tellapart.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/
-----------------------------------------------------------
(Updated Feb. 25, 2015, 9:22 p.m.)
Review request for Aurora, Brian Wickman and Zameer Manji.
Repository: aurora
Description (updated)
-------
Stop the announcer and status checkers before starting to kill the runners.
This allows the task to be removed from the ZK ensemble before it begins getting killed. The delay can be significant if the task takes some time to shutdown, and during the time it stops responding to requests.
Diffs
-----
src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
Diff: https://reviews.apache.org/r/31423/diff/
Testing
-------
We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
I couldn't observe any other side effects either.
Thanks,
Steve Niemitz
Re: Review Request 31423: Stop the announcer and status checkers
before starting to kill the runners
Posted by Steve Niemitz <st...@tellapart.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31423/
-----------------------------------------------------------
(Updated Feb. 25, 2015, 9:16 p.m.)
Review request for Aurora, Brian Wickman and Zameer Manji.
Repository: aurora
Description
-------
Stop the announcer and status checkers before starting to kill the runners
Diffs
-----
src/main/python/apache/aurora/executor/aurora_executor.py 9c0282392dbb9cca308baf47adc1750c1f5cacc6
Diff: https://reviews.apache.org/r/31423/diff/
Testing (updated)
-------
We're now running this in our production environments. Watching ZK, I can confirm that the nodes are removed before process shutdown begins. Watching the executor log also confirms this.
I couldn't observe any other side effects either.
Thanks,
Steve Niemitz